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

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

I think that avoiding accessing the length property each time will speed up it a little more.
varl=array.length;varval;vari;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：

varutil=require('util');varcommon=require('../common.js');varbench=common.createBenchmark(main,{n: [5e6]});functionmain(conf){varn=conf.n|0;bench.start();for(vari=0;i<n;i+=1){varr=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 dc5bacbCompare November 22, 2015 14:10

Trott force-pushed the master branch from 1e896a6 to 082cc8dCompare 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

Uh oh!

Uh oh!

Conversation

JacksonTian commented Nov 22, 2015

Uh oh!

thefourtheye commented Nov 22, 2015

Uh oh!

JacksonTian commented Nov 22, 2015

Uh oh!

bnoordhuis commented Nov 22, 2015

Uh oh!

cjihrig commented Nov 22, 2015

Uh oh!

lpincaNov 22, 2015

Choose a reason for hiding this comment

Uh oh!

JacksonTianNov 22, 2015

Choose a reason for hiding this comment

Uh oh!

tflanaganNov 22, 2015

Choose a reason for hiding this comment

Uh oh!

yorkieNov 22, 2015

Choose a reason for hiding this comment

Uh oh!

mscdexNov 22, 2015

Choose a reason for hiding this comment

Uh oh!

JacksonTian commented Nov 22, 2015

Uh oh!

mscdex commented Nov 22, 2015

Uh oh!

bnoordhuis commented Nov 22, 2015

Uh oh!

bnoordhuis commented Nov 23, 2015

Uh oh!

evanlucas commented Dec 1, 2015

Uh oh!

JacksonTian commented Dec 14, 2015

Uh oh!

cjihrig commented Dec 14, 2015

Uh oh!

trevnorris commented Dec 28, 2015

Uh oh!

cjihrig commented Dec 29, 2015

Uh oh!

jasnell commented Dec 30, 2015

Uh oh!

jasnell commented Dec 30, 2015

Uh oh!

jasnell commented Dec 30, 2015

Uh oh!

jbergstroem commented Dec 31, 2015

Uh oh!

jasnell commented Dec 31, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants