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 — stream: improve `Readable#from` perf by rluvaton · Pull Request #50359 · nodejs/node

Conversation

@rluvaton

@rluvaton rluvaton commented Oct 24, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Benchmarks

Benchmark URL

15:39:36                                                                             confidence improvement accuracy (*)    (**)   (***)
15:39:36 streams/readable-from.js type='array' n=10000000                                   ***     23.20 %       ±9.27% ±12.34% ±16.09%
15:39:36 streams/readable-from.js type='async-generator' n=10000000                           *      2.28 %       ±1.83%  ±2.44%  ±3.17%
15:39:36 streams/readable-from.js type='sync-generator-with-async-values' n=10000000                 0.48 %       ±1.82%  ±2.42%  ±3.15%
15:39:36 streams/readable-from.js type='sync-generator-with-sync-values' n=10000000         ***     18.26 %       ±2.01%  ±2.68%  ±3.49%

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, H4ad, and MomenNano reacted with rocket emoji
All reactions
  • 🚀 3 reactions
@rluvaton rluvaton added stream Issues and PRs related to Node.js streams. performance Issues and PRs related to the performance of Node.js. labels Oct 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 24, 2023
@rluvaton rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 24, 2023
@rluvaton

This comment was marked as outdated.

Sign in to view
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55177/
All reactions

Sorry, something went wrong.

Uh oh!

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

ronag commented Oct 24, 2023

Copy link
Copy Markdown
Member

Not sure I'm convinced the extra complexity is worth it. Let's see what the benchmark says.
All reactions

Sorry, something went wrong.

Uh oh!

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

ronag commented Oct 24, 2023

Copy link
Copy Markdown
Member

@rluvaton when you start a benchmark ci in cases like this, add a filter e,g, from so not all benchmarks run.
rluvaton and MoLow reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

ronag commented Oct 24, 2023

Copy link
Copy Markdown
Member

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1457/
rluvaton reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

ronag commented Oct 24, 2023

Copy link
Copy Markdown
Member 
11:20:15 streams/readable-from.js type='array' n=10000000                                            7.03 %       ±7.37% ±9.91% ±13.11%
11:20:15 streams/readable-from.js type='async-generator' n=10000000                           *      1.64 %       ±1.41% ±1.88%  ±2.45%
11:20:15 streams/readable-from.js type='sync-generator-with-async-values' n=10000000                 1.62 %       ±1.68% ±2.24%  ±2.92%
11:20:15 streams/readable-from.js type='sync-generator-with-sync-values' n=10000000         ***     11.09 %       ±1.82% ±2.42%  ±3.16%
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag reviewed Oct 24, 2023
View reviewed changes
Comment thread lib/internal/streams/from.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag reviewed Oct 24, 2023
View reviewed changes
Comment thread lib/internal/streams/from.js Resolved

Uh oh!

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

ronag commented Oct 24, 2023

Copy link
Copy Markdown
Member

I'm -0. There is a perf improvement but the maintainability cost is quite high.
All reactions

Sorry, something went wrong.

Uh oh!

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

rluvaton commented Oct 24, 2023

Copy link
Copy Markdown
Member Author

due to the removal of the async modifier I rerun the benchmark:
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1458/

I'm -0. There is a perf improvement but the maintainability cost is quite high.

I'm on the fence as well...
All reactions

Sorry, something went wrong.

Uh oh!

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

rluvaton commented Oct 24, 2023

Copy link
Copy Markdown
Member Author

@ronag Updated the benchmarks...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rluvaton rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 24, 2023
H4ad
H4ad approved these changes Oct 24, 2023
View reviewed changes
@H4ad H4ad added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 24, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55183/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag approved these changes Oct 24, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55193/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55194/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Oct 24, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55195/
All reactions

Sorry, something went wrong.

Uh oh!

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

benjamingr commented Oct 24, 2023

Copy link
Copy Markdown
Member

I'm tired so if I don't make sense let me know - In the array case can't we just set the stream's buffer to ArrayPrototypeSlice(Input)? With ronag's bufferliist->array PR that could be ideal?
MoLow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rluvaton
rluvaton force-pushed the improve-from branch from bdcda68 to aa3d6c2 Compare October 24, 2023 17:31
@rluvaton

rluvaton commented Oct 24, 2023

Copy link
Copy Markdown
Member Author

I'm tired so if I don't make sense let me know - In the array case can't we just set the stream's buffer to ArrayPrototypeSlice(Input)? With ronag's bufferliist->array PR that could be ideal?

it would really improve the performance for sure but wouldn't it be dangerous as we need to make the stream reach the state where it would have data after init and everything
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55228/
All reactions

Sorry, something went wrong.

Uh oh!

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

ronag commented Oct 25, 2023

Copy link
Copy Markdown
Member

Wouldn't we skip a lot of state transitions that the user expects?

Not really?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
H4ad
H4ad approved these changes Oct 25, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55233/
All reactions

Sorry, something went wrong.

Uh oh!

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

benjamingr commented Oct 25, 2023

Copy link
Copy Markdown
Member

In that case this whole implementation would basically be:

Yeah that was my point in #50359 I think that would be way faster and probably semver-major
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rluvaton rluvaton added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 25, 2023
@rluvaton

rluvaton commented Oct 25, 2023

Copy link
Copy Markdown
Member Author

In that case this whole implementation would basically be:

Yeah that was my point in #50359 I think that would be way faster and probably semver-major

Because it needs a semver major, I will create a new PR with the new code after that
benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rluvaton rluvaton added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Oct 25, 2023
@rluvaton
stream: improve from perf
ae6dd74
@rluvaton
rluvaton force-pushed the improve-from branch from e5352d6 to ae6dd74 Compare October 25, 2023 21:56
@rluvaton rluvaton added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Oct 25, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 25, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55237/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
H4ad
H4ad approved these changes Oct 25, 2023
View reviewed changes
@github-actions github-actions Bot mentioned this pull request Oct 26, 2023
CI Reliability 2023-10-26 nodejs/reliability#698
Open
20 tasks
@nodejs-github-bot

nodejs-github-bot commented Oct 26, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55246/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 26, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 10d51e8 into nodejs:main Oct 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in 10d51e8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rluvaton
rluvaton deleted the improve-from branch October 26, 2023 09:04
This was referenced Oct 27, 2023
CI Reliability 2023-10-27 nodejs/reliability#699
Open
CI Reliability 2023-10-28 nodejs/reliability#700
Open
CI Reliability 2023-10-29 nodejs/reliability#701
Open
CI Reliability 2023-10-30 nodejs/reliability#702
Open
targos pushed a commit that referenced this pull request Nov 11, 2023
@rluvaton @targos
stream: improve from perf
e20b272 
PR-URL: #50359
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@targos targos mentioned this pull request Nov 12, 2023
v21.2.0 release proposal #50681
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@rluvaton @UlisesGascon
stream: improve from perf
08a0c6c 
PR-URL: #50359
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@ronag ronag ronag approved these changes
@H4ad H4ad H4ad approved these changes

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. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. stream Issues and PRs related to Node.js streams.

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 @nodejs-github-bot @ronag @benjamingr @H4ad
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.