Skip to content

Conversation

@H4ad
Copy link
Member

@H4adH4ad 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-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 20, 2023
@anonriganonrig 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
@anonriganonrig 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-actionsgithub-actionsbot 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

@rluvaton
Copy link
Member

I'm happy to see my PR inspired this 😀

Copy link
Member

@RafaelGSSRafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM

@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@github-actionsgithub-actionsbot 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
MemberAuthor

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
MemberAuthor

H4ad commented Sep 22, 2023

This piece of code:

constbuff=Buffer.allocUnsafe(1024**2);constsource=newFile(buff,'dummy.txt',options);constnow=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.

@aduh95aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 29, 2023
@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2023
@nodejs-github-botnodejs-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 <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Oct 4, 2023
PR-URL: #49730 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Oct 4, 2023
PR-URL: #49730 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
@H4adH4ad deleted the perf/faster-blob branch October 4, 2023 01:13
@tniessen
Copy link
Member

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

@anonrig
Copy link
Member

@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

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

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 <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49730 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49730 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49730 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
@targostargos added dont-land-on-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 <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
@targostargos mentioned this pull request Nov 12, 2023
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49730 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49730 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49730 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.dont-land-on-v20.xPRs that should not land on the v20.x-staging branch and should not be released in v20.x.needs-benchmark-ciPR that need a benchmark CI run.needs-ciPRs that need a full CI run.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

14 participants

@H4ad@nodejs-github-bot@rluvaton@RafaelGSS@aduh95@Uzlopak@tniessen@anonrig@Qard@benjamingr@himself65@Ethan-Arrowood@KhafraDev@targos