-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
stream: improve ReadableStream.tee
perf by remove ReflectConstruct
usage
#49546
stream: improve ReadableStream.tee
perf by remove ReflectConstruct
usage
#49546
Conversation
also added more webstream creation benchmarks
// For spec compliance the Tee must be a ReadableStream | ||
tee.constructor = ReadableStream; |
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.
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
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.
You beat me to it. Good work!
@@ -1199,34 +1199,41 @@ ObjectDefineProperties(ReadableByteStreamController.prototype, { | |||
[SymbolToStringTag]: getNonWritablePropertyDescriptor(ReadableByteStreamController.name), | |||
}); | |||
|
|||
function TeeReadableStream(start, pull, cancel) { |
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.
is there a reason this can't be a normal class? Would help with the inheritance/prototype hacks needed
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'll have to call super and the original implementation don't call the constructor
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 think it would simplify the code significantly, not blocking or anything
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.
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
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.
lgtm
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: 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: nodejs#49546 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
also added more webstream creation benchmarks
My local benchmark shows improvement of 2 times:
After:
Benchmark CI output: