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

buffer: improve copy() performance #29066

Closed
wants to merge 1 commit into from

Conversation

@mscdex
Copy link
Contributor

commented Aug 9, 2019

Results of included benchmark:

                                                               confidence improvement accuracy (*)   (**)   (***)
 buffers/buffer-copy.js n=6000000 partial='false' bytes=0            ***    580.96 %       ±3.05% ±4.06%  ±5.30%
 buffers/buffer-copy.js n=6000000 partial='false' bytes=8            ***    179.90 %       ±5.71% ±7.69% ±10.20%
 buffers/buffer-copy.js n=6000000 partial='false' bytes=128          ***    174.81 %       ±1.17% ±1.56%  ±2.05%
 buffers/buffer-copy.js n=6000000 partial='false' bytes=32768          *      3.62 %       ±3.30% ±4.44%  ±5.89%
 buffers/buffer-copy.js n=6000000 partial='true' bytes=0             ***    566.67 %       ±3.63% ±4.87%  ±6.42%
 buffers/buffer-copy.js n=6000000 partial='true' bytes=8             ***     24.66 %       ±1.17% ±1.57%  ±2.05%
 buffers/buffer-copy.js n=6000000 partial='true' bytes=128           ***     19.53 %       ±0.61% ±0.81%  ±1.06%
 buffers/buffer-copy.js n=6000000 partial='true' bytes=32768                 -0.52 %       ±0.98% ±1.31%  ±1.70%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

@ronag ronag referenced this pull request Aug 9, 2019
2 of 2 tasks complete
@YurySolovyov

This comment has been minimized.

Copy link

commented Aug 10, 2019

any results for >32k?

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

@YurySolovyov No, is there a particular value you have in mind that you would like to see?

@YurySolovyov

This comment has been minimized.

Copy link

commented Aug 11, 2019

I just wonder if it actually degrades for larger inputs

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

FWIW I just tried with 1MB and it seems only then is there a slight regression (~1%) when doing a partial copy. The funny thing is that even deferring to C++ with no parameter/value validation (basically just a memset()) in that particular case, it slows things down significantly more than the original regression.

My guess is that there are missing optimizations in V8 for some larger values?

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

@Trott
Trott approved these changes Aug 14, 2019
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Landed in 6d351d4

@Trott Trott closed this Aug 14, 2019

Trott added a commit to Trott/io.js that referenced this pull request Aug 14, 2019
buffer: improve copy() performance
PR-URL: nodejs#29066
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@mscdex mscdex deleted the mscdex:buffer-copy-perf branch Aug 14, 2019

targos added a commit that referenced this pull request Aug 19, 2019
buffer: improve copy() performance
PR-URL: #29066
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Aug 19, 2019
buffer: improve copy() performance
PR-URL: #29066
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos referenced this pull request Aug 19, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
buffer: improve copy() performance
PR-URL: nodejs#29066
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
buffer: improve copy() performance
PR-URL: nodejs#29066
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.