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

benchmark: shorten pipe-to by reducing number of chunks #49577

Merged

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Sep 9, 2023

(this only now start taking a long time due to #49552)

without this, it takes 2.5 hours with this it should take around 15 minutes:

each iteration (single combination) takes 9 seconds (saw here).

there are 16 possible combinations.
so 9 seconds * 16 combination * 30 default runs * 2 versions = 2 hours and 24 minutes...

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Sep 9, 2023
@rluvaton
Copy link
Member Author

rluvaton commented Sep 9, 2023

Benchmark link (with 2 iterations to see the general time it should take): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1371/

Edit: each combination takes 1 second so it should now take around 15m

@mscdex
Copy link
Contributor

mscdex commented Sep 9, 2023

FWIW you shouldn't always reduce iterations just to make them take less time.

In most cases the number of iterations is where it is for a reason, namely because V8 does not optimize code right away (or the opposite: it could eventually deoptimize code for some reason) so you can easily get misleading results if the process does not stay alive long enough.

@rluvaton
Copy link
Member Author

rluvaton commented Sep 9, 2023

Do you think it's applicable here? Because 2.5 hours is way too long

@mscdex
Copy link
Contributor

mscdex commented Sep 9, 2023

Do you think it's applicable here?

I would say it's always applicable because it's not really safe to guess what V8 may or may not do, especially across V8 versions.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

I'm +1

Copy link
Member

@debadree25 debadree25 left a comment

Choose a reason for hiding this comment

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

+1 as well, ran into the same trying to run the entire suite of webstreams benchmark locally

@debadree25 debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2023
@rluvaton rluvaton added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 13, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 13, 2023
@nodejs-github-bot nodejs-github-bot merged commit 7ad4c0f into nodejs:main Sep 13, 2023
28 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7ad4c0f

@rluvaton rluvaton deleted the make-pipe-to-benchmark-shorter branch September 13, 2023 21:03
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
PR-URL: #49577
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
This was referenced Sep 28, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49577
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.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. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants