buffer: improve toJSON() performance by mscdex · Pull Request #10895 · nodejs/node

mscdex · 2017-01-19T08:25:31Z

Here are the results with the new, included benchmarks (I used a different n value for each input length for best test result reliability while making sure they finished in a reasonable amount of time):

                                           improvement confidence      p.value
 buffers/buffer-tojson.js len=0 n=10000000   2449.70 %        *** 1.312673e-26
 buffers/buffer-tojson.js len=10 n=1000000   3148.31 %        *** 1.136735e-13
 buffers/buffer-tojson.js len=256 n=100000   2717.46 %        *** 1.418324e-21
 buffers/buffer-tojson.js len=4096 n=10000   3121.78 %        *** 2.456344e-12

CI: https://ci.nodejs.org/job/node-test-pull-request/5945/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

fhalde · 2017-01-19T08:33:23Z

benchmark/buffers/buffer-tojson.js

+});
+
+function main(conf) {
+  var n = +conf.n;


why not use let or const for n and buf to maintain consistency across the codebase

Personally I'd rather just stick to var in benchmark code for the forseeable future. Having issues with let and const in node core code has left a bit of a "bad taste." While it might seem moot in benchmark code, I'd rather not have to worry about deopts making my benchmarks run unnecessarily longer.

In addition to what @mscdex said, the current position on const/let over var is "only in code where performance doesn't matter". I'd say it makes more sense for benchmarks/ to be consistent with lib/ than test/.

oh that was something new! Thanks!

evanlucas

LGTM with a question

evanlucas · 2017-01-19T12:03:57Z

lib/buffer.js

-    data: Array.prototype.slice.call(this, 0)
-  };
+  if (this.length) {
+    const data = [];


Is this faster than using new Array(this.length)?

It is until appropriate backports to V8 5.4 are made. I've just decided to go ahead and use this method. See this issue for more information.

ronkorving · 2017-01-19T12:51:20Z

lib/buffer.js

+      data[i] = this[i];
+    return { type: 'Buffer', data };
+  } else {
+    return { type: 'Buffer', data: [] };


Why the special case at all? The loop would run 0 times if length is 0, and you would get exactly the output you want. I doubt performance would be any different, would it?

Unless the difference is really noticeable, I think I'd rather keep this simple and drop the if statement as @ronkorving said.

There is a noticeable performance difference, otherwise I wouldn't have bothered. Without checking the IR explicitly, it probably has to do with setting up the loop or similar.

lpinca · 2017-01-23T11:30:27Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/5992/

lpinca · 2017-01-23T12:29:23Z

Landed in 9fcd842.

PR-URL: #10895 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#10895 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-03-08T02:19:22Z

@mscdex how long do you think this should bake before we land it in LTS?

mscdex · 2017-03-08T02:36:35Z

@MylesBorins In general I don't have an opinion on such wait times, but this is definitely a pretty straight forward, simple change, so as far as I'm concerned it could be backported at any time.

PR-URL: #10895 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) #10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) #10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) #11522 - `os.loadavg` is up to 151% faster. (Brian White) #11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) #11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) #10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) #10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) #10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) #11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) #9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) #10522 PR-URL: #11759

Notable Changes: * buffer: - The performance of `.toJSON()` is now up to 2859% faster on average (Brian White) #10895 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186

Notable Changes: * buffer: - The performance of `.toJSON()` is now up to 2859% faster on average (Brian White) #10895 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 PR-URL: #11760

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) #10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) #10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) #11522 - `os.loadavg` is up to 151% faster. (Brian White) #11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) #11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) #10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) #10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) #10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) #11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) #9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) #10522 PR-URL: #11759

mihailik · 2017-03-22T11:49:48Z

2.5 thousand percent!
🥇

Notable Changes: * buffer: - The performance of `.toJSON()` is now up to 2859% faster on average (Brian White) nodejs/node#10895 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) nodejs/node#10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) nodejs/node#10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) nodejs/node#11522 - `os.loadavg` is up to 151% faster. (Brian White) nodejs/node#11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) nodejs/node#11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) nodejs/node#10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) nodejs/node#10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) nodejs/node#10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) nodejs/node#11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) nodejs/node#9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) nodejs/node#10522 PR-URL: nodejs/node#11759 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * buffer: - The performance of `.toJSON()` is now up to 2859% faster on average (Brian White) nodejs/node#10895 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) nodejs/node#10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) nodejs/node#10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) nodejs/node#11522 - `os.loadavg` is up to 151% faster. (Brian White) nodejs/node#11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) nodejs/node#11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) nodejs/node#10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) nodejs/node#10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) nodejs/node#10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) nodejs/node#11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) nodejs/node#9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) nodejs/node#10522 PR-URL: nodejs/node#11759 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

mscdex added buffer Issues and PRs related to the buffer subsystem. lts-watch-v4.x performance Issues and PRs related to the performance of Node.js. labels Jan 19, 2017

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. lts-watch-v6.x labels Jan 19, 2017

buffer: improve toJSON() performance

a437f73

mscdex force-pushed the buffer-tojson-perf branch from 819dc38 to a437f73 Compare January 19, 2017 08:26

nodejs-github-bot added lts-watch-v6.x labels Jan 19, 2017

lpinca approved these changes Jan 19, 2017

View reviewed changes

fhalde reviewed Jan 19, 2017

View reviewed changes

targos approved these changes Jan 19, 2017

View reviewed changes

evanlucas approved these changes Jan 19, 2017

View reviewed changes

ronkorving reviewed Jan 19, 2017

View reviewed changes

cjihrig approved these changes Jan 19, 2017

View reviewed changes

jasnell approved these changes Jan 19, 2017

View reviewed changes

lpinca closed this Jan 23, 2017

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

mscdex deleted the buffer-tojson-perf branch January 25, 2017 07:26

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 8, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

Uh oh!

Conversation

mscdex commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

fhalde Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

fhalde Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

evanlucas left a comment

Choose a reason for hiding this comment

Uh oh!

evanlucas Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

mscdex Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

ronkorving Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

mscdex Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Jan 23, 2017

Uh oh!

lpinca commented Jan 23, 2017

Uh oh!

MylesBorins commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Mar 8, 2017

Uh oh!

mihailik commented Mar 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

mscdex commented Jan 19, 2017 •

edited

Loading

fhalde Jan 19, 2017 •

edited

Loading

mscdex Jan 19, 2017 •

edited

Loading

MylesBorins commented Mar 8, 2017 •

edited

Loading