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.

Sign up for GitHub

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

Jump to bottom

buffer: improve equals() performance #29199

Closed
wants to merge 1 commit into from
Closed

buffer: improve equals() performance #29199

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Aug 19, 2019

Results:

                                                            confidence improvement accuracy (*)   (**)  (***)
 buffers/buffer-equals.js n=10000000 difflen='false' size=0          ***    520.06 %       ±1.38%  ±1.85%   ±2.44%
 buffers/buffer-equals.js n=10000000 difflen='true' size=0           ***    674.98 %       ±2.18%  ±2.92%   ±3.85%
 buffers/buffer-equals.js n=10000000 difflen='true' size=16          ***    874.14 %       ±5.80%  ±7.81%  ±10.36%
 buffers/buffer-equals.js n=10000000 difflen='true' size=512         ***   1072.12 %       ±6.67%  ±8.98%  ±11.92%
 buffers/buffer-equals.js n=10000000 difflen='true' size=4096        ***   2569.30 %       ±4.33%  ±5.83%   ±7.74%
 buffers/buffer-equals.js n=4000000 difflen='true' size=16386        ***   7526.00 %      ±32.02% ±43.15%  ±57.28%

I'm not sure offhand what's up with the large variance for size=16386, especially since trying the same n value as the other cases actually resulted in an even larger variance. Anyway, the actual numbers don't really matter so much as the trend.

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

@mscdex mscdex added buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js. labels Aug 19, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@mscdex mscdex force-pushed the buffer-equals-perf branch from cf617f4 to 8ffa33f Compare August 19, 2019 05:52
@nodejs-github-bot
Copy link
Collaborator

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

BridgeAR
BridgeAR reviewed Aug 19, 2019
View reviewed changes
benchmark/buffers/buffer-equals.js Outdated
const common = require('../common.js');

const bench = common.createBenchmark(main, {
size: [0, 16, 512, 4096, 16386],
Copy link
Member

Choose a reason for hiding this comment

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

It should be sufficient to just keep e.g., 0, 512 and 16386. The other options are just intermediate steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I copied this from buffer-compare.js, does that mean that should be changed as well?

Copy link
Member

Choose a reason for hiding this comment

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

That would IMO be ideal 👍

@Trott
Copy link
Member

Trott commented Aug 21, 2019

Need to add difflen to test/benchmark/test-benchmark-buffer so that it still passes?

@Trott
Copy link
Member

Trott commented Aug 21, 2019

Need to add difflen to test/benchmark/test-benchmark-buffer so that it still passes?

Confirmed. This is needed. I tried to add a fixup commit but I don't have permission to the branch.

Trott
Trott requested changes Aug 21, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM but needs a change to avoid breaking test-benchmark-buffer

@mscdex
Copy link
Contributor Author

mscdex commented Aug 21, 2019

Shouldn't the test fail if it was breaking?

@mscdex mscdex force-pushed the buffer-equals-perf branch from 8ffa33f to f3f3b97 Compare August 21, 2019 14:57
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Aug 21, 2019 •
edited
Loading

Shouldn't the test fail if it was breaking?

benchmark tests only run nightly in CI and only if you run make test-all locally. I'd be +1 on running them a bit more widely, but we scaled it back when people complained about tests taking too long and benchmark test in particular being unreliable in certain environments, IIRC.

Trott
Trott reviewed Aug 21, 2019
View reviewed changes
test/benchmark/test-benchmark-buffer.js Outdated
@@ -26,6 +26,7 @@ runBenchmark('buffers',
'source=array',
'type=',
'value=0',
'withTotalLength=0'
'withTotalLength=0',
'difflen=false'
Copy link
Member

Choose a reason for hiding this comment

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

Super minor optional nit: The rest of the options are alphabetized. Can this go after line 14 intstead?

Trott
Trott approved these changes Aug 21, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with or without my additional optional nit

@mscdex mscdex force-pushed the buffer-equals-perf branch from f3f3b97 to c7fe371 Compare August 22, 2019 00:36
@mscdex mscdex force-pushed the buffer-equals-perf branch from c7fe371 to 36f9f73 Compare August 22, 2019 00:36
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Aug 22, 2019

Landed in f0c8898

@Trott Trott closed this Aug 22, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 22, 2019
@mscdex @Trott
buffer: improve equals() performance
f0c8898 
PR-URL: nodejs#29199
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@mscdex mscdex deleted the buffer-equals-perf branch August 22, 2019 12:51
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
@mscdex @BridgeAR
buffer: improve equals() performance
35bca31 
PR-URL: #29199
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@ZYSzys ZYSzys ZYSzys approved these changes

@trivikr trivikr trivikr approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mscdex @nodejs-github-bot @Trott @jasnell @lpinca @targos @BridgeAR @trivikr @ZYSzys