stream: improve `Readable#from` perf by rluvaton · Pull Request #50359 · nodejs/node

rluvaton · 2023-10-24T08:40:50Z

Benchmarks

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%

nodejs-github-bot · 2023-10-24T08:40:55Z

Review requested:

@nodejs/streams

nodejs-github-bot · 2023-10-24T08:44:50Z

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

ronag · 2023-10-24T08:50:29Z

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

ronag · 2023-10-24T08:51:58Z

@rluvaton when you start a benchmark ci in cases like this, add a filter e,g, from so not all benchmarks run.

ronag · 2023-10-24T08:53:21Z

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1457/

ronag · 2023-10-24T09:23:50Z

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%

ronag · 2023-10-24T09:27:35Z

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

rluvaton · 2023-10-24T12:13:12Z

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

rluvaton · 2023-10-24T12:49:33Z

@ronag Updated the benchmarks...

nodejs-github-bot · 2023-10-24T13:03:47Z

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

nodejs-github-bot · 2023-10-24T14:54:59Z

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

nodejs-github-bot · 2023-10-24T15:42:31Z

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

nodejs-github-bot · 2023-10-24T16:40:50Z

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

benjamingr · 2023-10-24T16:42:37Z

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?

rluvaton · 2023-10-24T17:35:14Z

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

nodejs-github-bot · 2023-10-25T12:51:42Z

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

ronag · 2023-10-25T13:00:33Z

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

Not really?

nodejs-github-bot · 2023-10-25T16:46:59Z

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

benjamingr · 2023-10-25T17:59:45Z

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

rluvaton · 2023-10-25T20:38:10Z

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

nodejs-github-bot · 2023-10-25T21:59:53Z

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

nodejs-github-bot · 2023-10-26T04:24:18Z

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

nodejs-github-bot · 2023-10-26T08:42:59Z

Landed in 10d51e8

PR-URL: #50359 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

rluvaton added stream Issues and PRs related to the stream subsystem. performance Issues and PRs related to the performance of Node.js. labels Oct 24, 2023

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 24, 2023

rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023

This comment was marked as outdated.
Sign in to view

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023

ronag reviewed Oct 24, 2023
View reviewed changes

Comment threadlib/internal/streams/from.js Outdated

ronag reviewed Oct 24, 2023
View reviewed changes

Comment threadlib/internal/streams/from.js Outdated

rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023

H4ad approved these changes Oct 24, 2023
View reviewed changes

H4ad added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023

ronag approved these changes Oct 24, 2023
View reviewed changes

benjamingr approved these changes Oct 24, 2023
View reviewed changes

rluvaton force-pushed the improve-from branch from bdcda68 to aa3d6c2Compare October 24, 2023 17:31

H4ad approved these changes Oct 25, 2023
View reviewed changes

rluvaton added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2023

rluvaton added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 25, 2023

stream: improve from perf
ae6dd74

rluvaton force-pushed the improve-from branch from e5352d6 to ae6dd74Compare October 25, 2023 21:56

rluvaton added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 25, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2023

H4ad approved these changes Oct 25, 2023
View reviewed changes

github-actionsBot mentioned this pull request Oct 26, 2023
CI Reliability 2023-10-26 nodejs/reliability#698
Open
20 tasks

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2023

nodejs-github-bot merged commit 10d51e8 into nodejs:mainOct 26, 2023

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
stream: improve from perf
e20b272
PR-URL: #50359 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

targos mentioned this pull request Nov 12, 2023
v21.2.0 release proposal #50681
Merged

UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
Merged

Uh oh!

Uh oh!

Conversation

rluvaton commented Oct 24, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

nodejs-github-bot commented Oct 24, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Oct 24, 2023

Uh oh!

ronag commented Oct 24, 2023

Uh oh!

ronag commented Oct 24, 2023

Uh oh!

ronag commented Oct 24, 2023

Uh oh!

ronag commented Oct 24, 2023

Uh oh!

Uh oh!

Uh oh!

ronag commented Oct 24, 2023

Uh oh!

rluvaton commented Oct 24, 2023

Uh oh!

rluvaton commented Oct 24, 2023

Uh oh!

nodejs-github-bot commented Oct 24, 2023

Uh oh!

nodejs-github-bot commented Oct 24, 2023

Uh oh!

nodejs-github-bot commented Oct 24, 2023

Uh oh!

nodejs-github-bot commented Oct 24, 2023

Uh oh!

benjamingr commented Oct 24, 2023

Uh oh!

rluvaton commented Oct 24, 2023

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

ronag commented Oct 25, 2023

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

benjamingr commented Oct 25, 2023

Uh oh!

rluvaton commented Oct 25, 2023

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

nodejs-github-bot commented Oct 26, 2023

Uh oh!

nodejs-github-bot commented Oct 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rluvaton commented Oct 24, 2023•
edited
Loading