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

lib: faster internal createBlob #49730

Closed
wants to merge 7 commits into from
Closed

Conversation

H4ad
Copy link
Member

@H4ad H4ad commented Sep 20, 2023

Inspired by nodejs/performance#109, I removed the usage of ReflectConstruct which is very heavy to call, and I also added a couple more benchmarks for blob.

Improvements:

                                           confidence improvement accuracy (*)     (**)    (***)
blob/creation.js kind='Blob' n=50000                       0.02 %       ±1.66%   ±2.21%   ±2.88%
blob/creation.js kind='createBlob' n=50000        ***   2727.18 %     ±138.63% ±186.82% ±248.02%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 2 comparisons, you can thus
expect the following amount of false-positive results:
  0.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.02 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)
                                 confidence improvement accuracy (*)    (**)   (***)
blob/resolveObjectURL.js n=50000        ***    304.19 %      ±11.73% ±15.80% ±20.95%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 1 comparisons, you can thus
expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)
                                    confidence improvement accuracy (*)    (**)   (***)
blob/slice.js dataSize=30720 n=50000        ***    445.71 %      ±32.71% ±44.06% ±58.44%
blob/slice.js dataSize=5 n=50000            ***    456.75 %      ±22.46% ±30.25% ±40.13%

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

I also think we can improve ClonedBlob but I tried using Function and also Class inheritance but I was not able to make it work correctly, but we probably can improve by 50% if we can make a version without ReflectConstruct.

cc @nodejs/performance

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 20, 2023
@anonrig anonrig added performance Issues and PRs related to the performance of Node.js. needs-benchmark-ci PR that need a benchmark CI run. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 20, 2023
@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

lib/internal/blob.js Show resolved Hide resolved
@rluvaton
Copy link
Member

I'm happy to see my PR inspired this 😀

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS
Copy link
Member

@rluvaton
Copy link
Member

It looks like the benchmark CI was stuck (nothing changed for 8 hours)

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

@H4ad
Copy link
Member Author

H4ad commented Sep 22, 2023

@rluvaton Can you cancel and start again? I lowered the values of the benchmark for blob and file, because of the regression found on nodejs/performance#118

@rluvaton
Copy link
Member

@H4ad
Copy link
Member Author

H4ad commented Sep 22, 2023

This piece of code:

const buff = Buffer.allocUnsafe(1024 ** 2);
const source = new File(buff, 'dummy.txt', options);

const now = performance.now();
source.text().then(() => console.log(`diff: ${performance.now() - now}`));

Takes 3s to run, and running 1e3 will take a looong time, I will update the benchmark again, sorry for that.

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 29, 2023
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 328bdac...1a839f3

nodejs-github-bot pushed a commit that referenced this pull request Oct 4, 2023
PR-URL: #49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
nodejs-github-bot pushed a commit that referenced this pull request Oct 4, 2023
PR-URL: #49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
nodejs-github-bot pushed a commit that referenced this pull request Oct 4, 2023
PR-URL: #49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@H4ad H4ad deleted the perf/faster-blob branch October 4, 2023 01:13
@tniessen
Copy link
Member

tniessen commented Oct 5, 2023

@anonrig FWIW, none of the three commits that were merged here adhere to our commit message guidelines.

@anonrig
Copy link
Member

anonrig commented Oct 5, 2023

@anonrig FWIW, none of the three commits that were merged here adhere to our commit message guidelines.

Is this a bug with node-core-utils? It seems we shouldn't even land commit that adhere.

@aduh95
Copy link
Contributor

aduh95 commented Oct 5, 2023

Reviewing the commit message is the burden of the PR author and the reviewers, NCU can only catch a subset of commit message "violation" – in this case, it would be very hard for NCU to guess that added, faster, and improved are not imperative verbs.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 5, 2023

The problem is that the length of the first commit message was reported to be too long. So something should have errored like when you dont prefix the First commit message with util: or so.

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@targos targos added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Nov 11, 2023
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49730
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.