GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — benchmark: shorten pipe-to by reducing number of chunks by rluvaton · Pull Request #49577 · nodejs/node

Conversation

@rluvaton

@rluvaton rluvaton commented Sep 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

(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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rluvaton
benchmark: shorten pipe-to by reducing number of chunks
3cc0db1
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure. label Sep 9, 2023
@rluvaton

rluvaton commented Sep 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Sep 9, 2023

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rluvaton

rluvaton commented Sep 9, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Sep 9, 2023

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Sep 10, 2023
View reviewed changes

@anonrig anonrig left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm +1

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
debadree25
debadree25 approved these changes Sep 12, 2023
View reviewed changes

@debadree25 debadree25 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@debadree25 debadree25 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 13, 2023
@rluvaton rluvaton added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 13, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 13, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 7ad4c0f into nodejs:main Sep 13, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 13, 2023

Copy link
Copy Markdown
Collaborator

Landed in 7ad4c0f
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@rluvaton @ruyadorno
benchmark: shorten pipe-to by reducing number of chunks
78a6c73 
PR-URL: #49577
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
This was referenced Sep 28, 2023
v20.7.1 proposal #49917
Closed
v20.8.0 proposal #49932
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
+1 more reviewer
@debadree25 debadree25 debadree25 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@rluvaton @mscdex @nodejs-github-bot @anonrig @debadree25
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.