Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: faster arrayToHash #3964

Closed
wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

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

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

Why not a Set?

@JacksonTian
Copy link
Contributor Author

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

@bnoordhuis
Copy link
Member

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

@cjihrig
Copy link
Contributor

cjihrig commented Nov 22, 2015

LGTM

@@ -161,9 +161,10 @@ function stylizeNoColor(str, styleType) {
function arrayToHash(array) {
var hash = Object.create(null);

array.forEach(function(val) {
for (var i = 0; i < array.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

V8 can speedup it with OSR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why make it do more work?

Copy link
Contributor

Choose a reason for hiding this comment

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

using let here might have a more clear scope definition?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

The `util.format()` is used frequently, make the method faster
is better.
@JacksonTian
Copy link
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
Copy link
Contributor

mscdex commented Nov 22, 2015

LGTM

@bnoordhuis
Copy link
Member

@bnoordhuis
Copy link
Member

Linking to nodejs/build#263.

@evanlucas
Copy link
Contributor

@JacksonTian
Copy link
Contributor Author

What's wrong with CI?

@cjihrig
Copy link
Contributor

cjihrig commented Dec 14, 2015

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

@trevnorris
Copy link
Contributor

Anything holding this up?

@cjihrig
Copy link
Contributor

cjihrig commented Dec 29, 2015

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

@jasnell
Copy link
Member

jasnell commented Dec 30, 2015

LGTM

@jasnell
Copy link
Member

jasnell commented Dec 30, 2015

Will get this landed.

jasnell pushed a commit that referenced this pull request Dec 30, 2015
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
Copy link
Member

jasnell commented Dec 30, 2015

Landed in 3e740ca

@jbergstroem
Copy link
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
Copy link
Member

jasnell commented Dec 31, 2015

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 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
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
* 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
* 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
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
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 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
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
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.