stream: improve `ReadableStream.tee` perf by remove `ReflectConstruct` usage by rluvaton · Pull Request #49546 · nodejs/node

rluvaton · 2023-09-07T22:58:05Z

also added more webstream creation benchmarks

My local benchmark shows improvement of 2 times:

After:

webstreams/creation.js kind="ReadableStream.tee" n=50000: 126,685.06978447069 # Before
webstreams/creation.js kind="ReadableStream.tee" n=50000: 242,524.97642257062 # After

Benchmark CI output:

00:22:52.357 ++ Rscript benchmark/compare.R
00:22:53.287                                                                         confidence improvement accuracy (*)    (**)   (***)
00:22:53.287 webstreams/creation.js kind='ReadableStream.tee' n=50000                       ***     86.53 %       ±3.55%  ±4.75%  ±6.24%
00:22:53.287 webstreams/creation.js kind='ReadableStream' n=50000                                    1.83 %       ±3.38%  ±4.50%  ±5.86%
00:22:53.287 webstreams/creation.js kind='ReadableStreamBYOBReader' n=50000                         -0.23 %       ±3.61%  ±4.81%  ±6.26%
00:22:53.287 webstreams/creation.js kind='ReadableStreamDefaultReader' n=50000                       9.73 %      ±11.91% ±15.87% ±20.70%
00:22:53.287 webstreams/creation.js kind='TransformStream' n=50000                                   0.96 %       ±2.82%  ±3.75%  ±4.89%
00:22:53.287 webstreams/creation.js kind='WritableStream' n=50000                                   -1.19 %       ±4.27%  ±5.68%  ±7.40%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=1024 n=5000000                -0.19 %       ±4.53%  ±6.03%  ±7.86%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=2048 n=5000000                -3.72 %       ±4.90%  ±6.53%  ±8.53%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=4096 n=5000000                 2.64 %       ±3.81%  ±5.07%  ±6.60%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=1024 highWaterMarkR=512 n=5000000                  3.33 %       ±4.26%  ±5.70%  ±7.49%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=1024 n=5000000                 0.78 %       ±4.03%  ±5.36%  ±6.98%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=2048 n=5000000                -1.02 %       ±3.54%  ±4.71%  ±6.14%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=4096 n=5000000                -4.02 %       ±4.44%  ±5.93%  ±7.75%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=2048 highWaterMarkR=512 n=5000000                 -2.21 %       ±3.80%  ±5.06%  ±6.60%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=1024 n=5000000                -1.65 %       ±3.45%  ±4.59%  ±5.98%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=2048 n=5000000                -1.65 %       ±3.83%  ±5.11%  ±6.66%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=4096 n=5000000                 1.25 %       ±3.83%  ±5.10%  ±6.64%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=4096 highWaterMarkR=512 n=5000000                 -1.78 %       ±4.88%  ±6.50%  ±8.46%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=1024 n=5000000                  3.39 %       ±5.70%  ±7.58%  ±9.87%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=2048 n=5000000                 -2.18 %       ±5.81%  ±7.74% ±10.11%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=4096 n=5000000                 -1.72 %       ±3.33%  ±4.43%  ±5.77%
00:22:53.287 webstreams/pipe-to.js highWaterMarkW=512 highWaterMarkR=512 n=5000000                   1.77 %       ±4.47%  ±5.96%  ±7.77%
00:22:53.382 webstreams/readable-async-iterator.js n=100000                                          1.66 %       ±7.72% ±10.27% ±13.36%
00:22:53.383 
00:22:53.383 Be aware that when doing many comparisons the risk of a false-positive
00:22:53.383 result increases. In this case, there are 23 comparisons, you can thus
00:22:53.383 expect the following amount of false-positive results:
00:22:53.383   1.15 false positives, when considering a   5% risk acceptance (*, **, ***),
00:22:53.383   0.23 false positives, when considering a   1% risk acceptance (**, ***),
00:22:53.383   0.02 false positives, when considering a 0.1% risk acceptance (***)

also added more webstream creation benchmarks

rluvaton · 2023-09-07T23:01:02Z

lib/internal/webstreams/readablestream.js

+  // For spec compliance the Tee must be a ReadableStream
+  tee.constructor = ReadableStream;


Otherwise, the wpt tee test fails:

node/test/fixtures/wpt/streams/readable-streams/tee.any.js

Lines 8 to 18 in 6dde810

test(() => {

const rs = new ReadableStream();

const result = rs.tee();

assert_true(Array.isArray(result), 'return value should be an array');

assert_equals(result.length, 2, 'array should have length 2');

assert_equals(result[0].constructor, ReadableStream, '0th element should be a ReadableStream');

assert_equals(result[1].constructor, ReadableStream, '1st element should be a ReadableStream');

}, 'ReadableStream teeing: rs.tee() returns an array of two ReadableStreams');

And the spec says

Tees this readable stream, returning a two-element array containing the two resulting branches as new ReadableStream instances.

From the Spec

rluvaton · 2023-09-07T23:02:49Z

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

nodejs-github-bot · 2023-09-07T23:05:41Z

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

nodejs-github-bot · 2023-09-07T23:14:02Z

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

anonrig

You beat me to it. Good work!

nodejs-github-bot · 2023-09-08T01:22:24Z

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

nodejs-github-bot · 2023-09-08T01:34:22Z

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

KhafraDev · 2023-09-08T02:56:03Z

lib/internal/webstreams/readablestream.js

  [SymbolToStringTag]: getNonWritablePropertyDescriptor(ReadableByteStreamController.name),
 });

+function TeeReadableStream(start, pull, cancel) {


is there a reason this can't be a normal class? Would help with the inheritance/prototype hacks needed

I'll have to call super and the original implementation don't call the constructor

I think it would simplify the code significantly, not blocking or anything

so you mean to just convert it to class with extends but without super?

when I do the WPT tests fails so keeping it as is

nodejs-github-bot · 2023-09-08T09:58:41Z

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

nodejs-github-bot · 2023-09-08T10:48:06Z

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

nodejs-github-bot · 2023-09-10T08:35:10Z

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

mcollina

lgtm

nodejs-github-bot · 2023-09-11T07:45:13Z

Landed in a7fe8b0

also added more webstream creation benchmarks PR-URL: nodejs#49546 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

also added more webstream creation benchmarks PR-URL: #49546 Backport-PR-URL: #49620 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

also added more webstream creation benchmarks PR-URL: nodejs#49546 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

stream: improve tee perf by reduce ReflectConstruct usages

76c567a

also added more webstream creation benchmarks

nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Sep 7, 2023

rluvaton added needs-benchmark-ci PR that need a benchmark CI run. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 7, 2023

rluvaton commented Sep 7, 2023

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2023

rluvaton added benchmark Issues and PRs related to the benchmark subsystem. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 7, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2023

rluvaton added the performance Issues and PRs related to the performance of Node.js. label Sep 7, 2023

anonrig approved these changes Sep 7, 2023

View reviewed changes

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

KhafraDev reviewed Sep 8, 2023

View reviewed changes

This was referenced Sep 9, 2023

CI Reliability 2023-09-09 nodejs/reliability#660

Open

CI Reliability 2023-09-10 nodejs/reliability#661

Open

H4ad mentioned this pull request Sep 10, 2023

Perf of ReflectConstruct nodejs/performance#109

Closed

github-actions bot mentioned this pull request Sep 11, 2023

CI Reliability 2023-09-11 nodejs/reliability#662

Open

18 tasks

mcollina approved these changes Sep 11, 2023

View reviewed changes

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

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

nodejs-github-bot merged commit a7fe8b0 into nodejs:main Sep 11, 2023

rluvaton deleted the add-benchmark-and-improve-webstream-creation-perf branch September 11, 2023 07:52

rluvaton mentioned this pull request Sep 11, 2023

v20.7.0 proposal #49592

Merged

rluvaton added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Sep 11, 2023

github-actions bot mentioned this pull request Sep 12, 2023

CI Reliability 2023-09-12 nodejs/reliability#663

Open

19 tasks

rluvaton mentioned this pull request Sep 12, 2023

[v20.x backport] stream: improve tee perf by reduce ReflectConstruct usages #49620

Closed

rluvaton added backport-open-v20.x Indicate that the PR has an open backport and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. labels Sep 12, 2023

targos added backported-to-v20.x PRs backported to the v20.x-staging branch. and removed backport-open-v20.x Indicate that the PR has an open backport labels Nov 12, 2023

		// For spec compliance the Tee must be a ReadableStream
		tee.constructor = ReadableStream;

	test(() => {

	const rs = new ReadableStream();
	const result = rs.tee();

	assert_true(Array.isArray(result), 'return value should be an array');
	assert_equals(result.length, 2, 'array should have length 2');
	assert_equals(result[0].constructor, ReadableStream, '0th element should be a ReadableStream');
	assert_equals(result[1].constructor, ReadableStream, '1st element should be a ReadableStream');

	}, 'ReadableStream teeing: rs.tee() returns an array of two ReadableStreams');

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rluvaton Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Sep 7, 2023

Uh oh!

nodejs-github-bot commented Sep 7, 2023

Uh oh!

nodejs-github-bot commented Sep 7, 2023

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 8, 2023

Uh oh!

nodejs-github-bot commented Sep 8, 2023

Uh oh!

KhafraDev Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

rluvaton Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

KhafraDev Sep 9, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 8, 2023

Uh oh!

nodejs-github-bot commented Sep 8, 2023

Uh oh!

nodejs-github-bot commented Sep 10, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rluvaton commented Sep 7, 2023 •

edited

Loading

rluvaton Sep 10, 2023 •

edited

Loading