util: faster arrayToHash #3964

JacksonTian · 2015-11-22T09:33:13Z

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

thefourtheye · 2015-11-22T10:17:53Z

Why not a Set?

JacksonTian · 2015-11-22T12:48:02Z

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

bnoordhuis · 2015-11-22T13:00:30Z

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

cjihrig · 2015-11-22T13:13:43Z

LGTM

lpinca · 2015-11-22T14:00:31Z

lib/util.js

@@ -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++) {


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

V8 can speedup it with OSR.

Why make it do more work?

using let here might have a more clear scope definition?

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 · 2015-11-22T14:09:18Z

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 · 2015-11-22T15:55:43Z

LGTM

bnoordhuis · 2015-11-22T18:28:14Z

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

bnoordhuis · 2015-11-23T17:55:03Z

Linking to nodejs/build#263.

evanlucas · 2015-12-01T02:16:21Z

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

JacksonTian · 2015-12-14T09:50:12Z

What's wrong with CI?

cjihrig · 2015-12-14T15:27:07Z

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

trevnorris · 2015-12-28T18:13:35Z

Anything holding this up?

cjihrig · 2015-12-29T16:41:59Z

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

jasnell · 2015-12-30T19:53:56Z

LGTM

jasnell · 2015-12-30T19:55:14Z

Will get this landed.

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 · 2015-12-30T20:00:21Z

Landed in 3e740ca

jbergstroem · 2015-12-31T07:36:49Z

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 · 2015-12-31T07:43:18Z

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).

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>

* 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 (José 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

* 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 (José 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>

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>

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>

* 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 (José 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>

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

lpinca reviewed Nov 22, 2015
View reviewed changes

util: faster arrayToHash

dc5bacb

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

JacksonTian force-pushed the fast_array_to_hash branch from 8b519cc to dc5bacb Compare November 22, 2015 14:10

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

jasnell closed this Dec 30, 2015

jasnell added the lts-watch-v4.x label Dec 30, 2015

JacksonTian deleted the fast_array_to_hash branch December 31, 2015 07:46

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 13, 2016

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: faster arrayToHash #3964

util: faster arrayToHash #3964

JacksonTian commented Nov 22, 2015

thefourtheye commented Nov 22, 2015

JacksonTian commented Nov 22, 2015

bnoordhuis commented Nov 22, 2015

cjihrig commented Nov 22, 2015

lpinca Nov 22, 2015

JacksonTian Nov 22, 2015

tflanagan Nov 22, 2015

yorkie Nov 22, 2015

mscdex Nov 22, 2015

JacksonTian commented Nov 22, 2015

mscdex commented Nov 22, 2015

bnoordhuis commented Nov 22, 2015

bnoordhuis commented Nov 23, 2015

evanlucas commented Dec 1, 2015

JacksonTian commented Dec 14, 2015

cjihrig commented Dec 14, 2015

trevnorris commented Dec 28, 2015

cjihrig commented Dec 29, 2015

jasnell commented Dec 30, 2015

jasnell commented Dec 30, 2015

jasnell commented Dec 30, 2015

jbergstroem commented Dec 31, 2015

jasnell commented Dec 31, 2015

util: faster arrayToHash #3964

util: faster arrayToHash #3964

Conversation

JacksonTian commented Nov 22, 2015

thefourtheye commented Nov 22, 2015

JacksonTian commented Nov 22, 2015

bnoordhuis commented Nov 22, 2015

cjihrig commented Nov 22, 2015

lpinca Nov 22, 2015

Choose a reason for hiding this comment

JacksonTian Nov 22, 2015

Choose a reason for hiding this comment

tflanagan Nov 22, 2015

Choose a reason for hiding this comment

yorkie Nov 22, 2015

Choose a reason for hiding this comment

mscdex Nov 22, 2015

Choose a reason for hiding this comment

JacksonTian commented Nov 22, 2015

mscdex commented Nov 22, 2015

bnoordhuis commented Nov 22, 2015

bnoordhuis commented Nov 23, 2015

evanlucas commented Dec 1, 2015

JacksonTian commented Dec 14, 2015

cjihrig commented Dec 14, 2015

trevnorris commented Dec 28, 2015

cjihrig commented Dec 29, 2015

jasnell commented Dec 30, 2015

jasnell commented Dec 30, 2015

jasnell commented Dec 30, 2015

jbergstroem commented Dec 31, 2015

jasnell commented Dec 31, 2015