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

stream: improve read() performance #28961

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Aug 4, 2019

Result:

                                                        confidence improvement accuracy (*)   (**)   (***)
 streams/readable-readall.js n=5000                           ***     85.06 %       ±2.87% ±3.84%  ±5.05%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

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

@ronag
Copy link
Member

ronag commented Aug 5, 2019

@mscdex Sorry to ask here. But, how do I generate/run that benchmark?

@mscdex
Copy link
Contributor Author

mscdex commented Aug 5, 2019

@ronag

Locally:

$ node benchmark/compare.js --new ./node --old ./node-master --filter readable-readall -- streams | Rscript benchmark/compare.R

or run it in benchmark CI and fill out some of the same parameters.

@ZYSzys ZYSzys added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 5, 2019
@Trott
Copy link
Member

Trott commented Aug 6, 2019

@ronag After running the benchmark as @mscdex describes, you'll want to process the results via compare.R to get the output similar to what @mscdex has in the PR here and probably what you've seen elsewhere in the issue tracker. Further details about benchmarks and the associated scripts are in https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md, especially https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md#comparing-nodejs-versions.

@Trott
Copy link
Member

Trott commented Aug 6, 2019

Oops, I see @mscdex included the R script at the end, so ignore that part of what I wrote. But the guide link may still come in handy.

@Trott
Copy link
Member

Trott commented Aug 7, 2019

Landed in 0b5b81c

@Trott Trott closed this Aug 7, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 7, 2019
PR-URL: nodejs#28961
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mscdex mscdex deleted the streams-read-perf branch August 7, 2019 05:35
targos pushed a commit that referenced this pull request Aug 19, 2019
PR-URL: #28961
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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. performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.