FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: faster arrayToHash by JacksonTian · Pull Request #3964 · nodejs/node · GitHub

/ node Public

util: faster arrayToHash - #3964

Closed
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:fast_array_to_hash
ConversationCommits (1)ChecksFiles changed
Closed

util: faster arrayToHash#3964
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:fast_array_to_hash

Conversation

JacksonTian commented Nov 22, 2015

Copy link
Copy Markdown
Contributor

The util.format() is used frequently, make the method faster
is better.

thefourtheye added the util Issues and PRs related to the built-in util module. label Nov 22, 2015

thefourtheye commented Nov 22, 2015

Copy link
Copy Markdown
Contributor

Why not a Set?

JacksonTian commented Nov 22, 2015

Copy link
Copy Markdown
Contributor Author

The result of arrayToHash was used by downstream with hasOwnProperty(). If use Set, need change more lines.

bnoordhuis commented Nov 22, 2015

Copy link
Copy Markdown
Member

LGTM but can you add a small benchmark in benchmark/misc and post before/after numbers?

cjihrig commented Nov 22, 2015

Copy link
Copy Markdown
Contributor

LGTM

lpinca reviewed Nov 22, 2015
View reviewed changes
Comment thread lib/util.js

lpinca Nov 22, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think that avoiding accessing the length property each time will speed up it a little more.

var l = array.length;
var val;
var i;

for (i = 0;  i < l; i++) {
  val = array[i];
  hash[val] = true;
}

JacksonTian Nov 22, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

V8 can speedup it with OSR.

tflanagan Nov 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why make it do more work?

yorkie Nov 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

using let here might have a more clear scope definition?

mscdex Nov 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

v8 is smart enough these days to cache the length automatically, so this shouldn't be an issue.

util: faster arrayToHash
dc5bacb 
The `util.format()` is used frequently, make the method faster
is better.

JacksonTian commented Nov 22, 2015

Copy link
Copy Markdown
Contributor Author

Benchmark is here：

var util = require('util');

var common = require('../common.js');

var bench = common.createBenchmark(main, {n: [5e6]});

function main(conf) {
  var n = conf.n | 0;

  bench.start();
  for (var i = 0; i < n; i += 1) {
    var r = util.inspect({a: 'a', b: 'b', c: 'c', d: 'd'});
  }
  bench.end(n);
}

before:
util/inspect.js n=5000000: 45599.83340

after:
util/inspect.js n=5000000: 63209.71860

cc @bnoordhuis

mscdex commented Nov 22, 2015

Copy link
Copy Markdown
Contributor

LGTM

bnoordhuis commented Nov 22, 2015

Copy link
Copy Markdown
Member

Thanks, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/810/

bnoordhuis commented Nov 23, 2015

Copy link
Copy Markdown
Member

Linking to nodejs/build#263.

evanlucas commented Dec 1, 2015

Copy link
Copy Markdown
Contributor

Trying CI again https://ci.nodejs.org/job/node-test-pull-request/887/

JacksonTian commented Dec 14, 2015

Copy link
Copy Markdown
Contributor Author

What's wrong with CI?

cjihrig commented Dec 14, 2015

Copy link
Copy Markdown
Contributor

Running CI again, just because it's been 2 weeks: https://ci.nodejs.org/job/node-test-pull-request/991/

trevnorris commented Dec 28, 2015

Copy link
Copy Markdown
Contributor

Anything holding this up?

cjihrig commented Dec 29, 2015

Copy link
Copy Markdown
Contributor

I don't think so. Running CI again, then hopefully this can land. https://ci.nodejs.org/job/node-test-pull-request/1106/

jasnell commented Dec 30, 2015

Copy link
Copy Markdown
Member

LGTM

jasnell commented Dec 30, 2015

Copy link
Copy Markdown
Member

Will get this landed.

jasnell pushed a commit that referenced this pull request Dec 30, 2015
util: faster arrayToHash
3e740ca 
The `util.format()` is used frequently, make the method faster
is better.

R-URL: #3964
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Dec 30, 2015

Copy link
Copy Markdown
Member

Landed in 3e740ca

jasnell closed this Dec 30, 2015

jbergstroem commented Dec 31, 2015

Copy link
Copy Markdown
Member

Minor heads up, typo in commit message: R-URL -> PR-URL. The grammar could have been slightly improved as well -- I would have liked reading what kind of speed improvement it brought.

jasnell commented Dec 31, 2015

Copy link
Copy Markdown
Member

Ugh. OK. Thanks for the heads up
On Dec 30, 2015 11:36 PM, "Johan Bergström" notifications@github.com
wrote:

Minor heads up, typo in commit message: R-URL -> PR-URL. The grammar
could have been slightly improved as well -- I would have liked reading
what kind of speed improvement it brought.

—
Reply to this email directly or view it on GitHub
#3964 (comment).

JacksonTian deleted the fast_array_to_hash branch December 31, 2015 07:46
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
util: faster arrayToHash
8039ca0 
The `util.format()` is used frequently, make the method faster
is better.

R-URL: nodejs#3964
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
2016-01-06, Version 5.4.0 (Stable)
af591d4 
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964

PR-URL: nodejs#4547
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 11, 2016
2016-01-06, Version 5.4.0 (Stable)
5f0abd6 
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons"
(Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F.
Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays
(Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris)
nodejs#3780, (Evan Lucas)
nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian)
nodejs#3964

Refs: nodejs#4547
PR-URL: nodejs#4623
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
util: faster arrayToHash
54bb2fb 
The `util.format()` is used frequently, make the method faster
is better.

R-URL: #3964
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
util: faster arrayToHash
f441410 
The `util.format()` is used frequently, make the method faster
is better.

R-URL: #3964
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 24, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
util: faster arrayToHash
20e2aa0 
The `util.format()` is used frequently, make the method faster
is better.

R-URL: nodejs#3964
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
2016-01-06, Version 5.4.0 (Stable)
3991aa8 
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons"
(Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F.
Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays
(Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris)
nodejs#3780, (Evan Lucas)
nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian)
nodejs#3964

Refs: nodejs#4547
PR-URL: nodejs#4623
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

performance Issues and PRs related to the performance of Node.js. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL