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: remove async tick and improve perf by 48% #44966

Closed
wants to merge 1 commit into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Oct 11, 2022

I ran the benchmarks only for blob.text() since it's the only relevant one, but added more for future reference.

                                                       confidence improvement accuracy (*)   (**)  (***)
blob/blob-text.js operation='text' n=1000000 bytes=0          ***     48.33 %       ±1.65% ±2.20% ±2.87%
blob/blob-text.js operation='text' n=1000000 bytes=128        ***      9.65 %       ±2.33% ±3.10% ±4.04%
blob/blob-text.js operation='text' n=1000000 bytes=8          ***      8.20 %       ±1.84% ±2.45% ±3.19%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 3 comparisons, you can thus expect the following amount of false-positive results:
  0.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.03 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 11, 2022
@RafaelGSS
Copy link
Member

@anonrig Did you run the benchmark against main or does it applies to other release lines too?

@anonrig
Copy link
Member Author

anonrig commented Oct 11, 2022

@RafaelGSS I run the command only on main using the following command: node benchmark/compare.js --old ./node-master --new ./out/Release/node blob > blob.csv. IMHO, I'm pretty sure it applies to all release lines.

benchmark/blob/blob-text.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Oct 11, 2022

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1195/

Results
                                                              confidence improvement accuracy (*)   (**)   (***)
blob/blob-text.js operation='arrayBuffer' n=1000000 bytes=0                  -0.15 %       ±5.14% ±6.85%  ±8.91%
blob/blob-text.js operation='arrayBuffer' n=1000000 bytes=128                -1.25 %       ±5.80% ±7.72% ±10.05%
blob/blob-text.js operation='arrayBuffer' n=1000000 bytes=8                  -2.79 %       ±5.23% ±6.97%  ±9.09%
blob/blob-text.js operation='stream' n=1000000 bytes=0                       -1.41 %       ±3.70% ±4.92%  ±6.41%
blob/blob-text.js operation='stream' n=1000000 bytes=128                     -1.43 %       ±2.57% ±3.42%  ±4.45%
blob/blob-text.js operation='stream' n=1000000 bytes=8                       -1.43 %       ±2.36% ±3.13%  ±4.08%
blob/blob-text.js operation='text' n=1000000 bytes=0                         -3.99 %       ±4.91% ±6.53%  ±8.50%
blob/blob-text.js operation='text' n=1000000 bytes=128                       -3.74 %       ±6.91% ±9.20% ±11.98%
blob/blob-text.js operation='text' n=1000000 bytes=8                         -2.47 %       ±3.00% ±4.00%  ±5.20%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 9 comparisons, you can thus
expect the following amount of false-positive results:
  0.45 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.09 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

@anonrig
Copy link
Member Author

anonrig commented Oct 11, 2022

I updated the benchmark result and the title according to the changes in the benchmark.

@anonrig anonrig changed the title buffer: remove async tick and improve perf by 680% buffer: remove async tick and improve perf by 48% Oct 11, 2022
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I am uncertain if it's really worth it as it will likely only improve the situation for very small entries that won't be the common case.
If it's really an API that is called millions of times with very small values, it's a different story. Using promise .then() has the downside of not providing async stack traces. Thus, I am -0.5.

const { Blob } = require('buffer');

const bench = common.createBenchmark(main, {
bytes: [0, 8, 128],
Copy link
Member

Choose a reason for hiding this comment

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

128 bytes seems rather small. I guess this is mainly used for bigger values such as 128kb, 1mb and similar.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@aduh95
Copy link
Contributor

aduh95 commented Oct 11, 2022

The benchmark CI didn't show any perf improvement with this PR 🤔

@anonrig
Copy link
Member Author

anonrig commented Oct 11, 2022

My branch was not up to date (by 35 commits). I rebased, built main & branch from scratch, force-pushed to this branch, removed stream from the benchmark, and re-run the benchmark on my machine. The results are similar. I don't have logical reasoning behind this difference. Can you re-run the benchmark CI @aduh95? If the results are the same, I'll just close the pull request.

@anonrig
Copy link
Member Author

anonrig commented Oct 11, 2022

Can we run the benchmark on v18 and v16 branches instead of main? I suspect v8 version bump fixed this regression.

@aduh95
Copy link
Contributor

aduh95 commented Oct 11, 2022

Benchmark CI (main): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1196/
Benchmark CI (v18.x): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1197/
Benchmark CI (v16.x): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1198/

N.B.: some of the above CIs are queued, the links will 404 until it starts.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

The benchmark shows no improvement:

23:50:37                                                               confidence improvement accuracy (*)   (**)   (***)
23:50:38 blob/blob-text.js operation='arrayBuffer' n=1000000 bytes=0                   1.80 %       ±4.26% ±5.68%  ±7.40%
23:50:38 blob/blob-text.js operation='arrayBuffer' n=1000000 bytes=128                -0.31 %       ±4.68% ±6.22%  ±8.10%
23:50:38 blob/blob-text.js operation='arrayBuffer' n=1000000 bytes=8                  -2.30 %       ±5.96% ±7.93% ±10.32%
23:50:38 blob/blob-text.js operation='text' n=1000000 bytes=0                         -3.52 %       ±5.14% ±6.83%  ±8.90%
23:50:38 blob/blob-text.js operation='text' n=1000000 bytes=128                        0.61 %       ±6.97% ±9.27% ±12.07%
23:50:38 blob/blob-text.js operation='text' n=1000000 bytes=8                         -1.03 %       ±2.79% ±3.71%  ±4.85%

The patch does not apply cleanly to older versions and can't automatically be checked against those versions due to that.

If this would really change anything, it would only be noticeable for super tiny blobs and they should not be of any concern. As such, I recommend to stick to what we have.

@anonrig
Copy link
Member Author

anonrig commented Oct 13, 2022

Sorry for the late reply. I rebuild this change on both v16 and v18, and I could not reproduce the same benchmark result on either branch. Therefore, I'm closing this pull request and opening a pull-request only for the benchmark: #44990.

I appreciate anyone who can help me understand how I got the initial 640% and 40% diff on benchmarks. Nevertheless, Thank you all for the reviews and comments.

@anonrig anonrig closed this Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants