Repository navigation
Performance of WHATWG ReadableStream.read() #82
Description
Activity
This could also probably impact fetch perf too!
Reacted by Yagiz NizipliRemoving primordials like
ArrayPrototypeShiftorArrayPrototypePushfrom the hot path led to a slight perf boost but not sure thats a good thing to doReacted by Matteo Collina- changed the title
[-]Performance of WHATWYG ReadableStream.read()[/-][+]Performance of WHATWG ReadableStream.read()[/+]on May 16, 2023 Getting rid of
ensureIsPromiseimproved perf by ~7.5% locally, but broke at least 2 tests from the WPTs. Maybe someone else will feel inspired:Details
diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 0b8b8ac1ef..9797a04c2c 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -46,6 +46,7 @@ const { const { isArrayBufferView, isDataView, + isPromise, } = require('internal/util/types'); const { @@ -2249,16 +2250,29 @@ function readableStreamDefaultControllerCallPullIfNeeded(controller) { } assert(!controller[kState].pullAgain); controller[kState].pulling = true; - PromisePrototypeThen( - ensureIsPromise(controller[kState].pullAlgorithm, controller), - () => { - controller[kState].pulling = false; - if (controller[kState].pullAgain) { - controller[kState].pullAgain = false; - readableStreamDefaultControllerCallPullIfNeeded(controller); - } - }, - (error) => readableStreamDefaultControllerError(controller, error)); + + function onPullResolve() { + controller[kState].pulling = false; + if (controller[kState].pullAgain) { + controller[kState].pullAgain = false; + readableStreamDefaultControllerCallPullIfNeeded(controller); + } + } + + try { + const value = FunctionPrototypeCall(controller[kState].pullAlgorithm, controller); + + if (isPromise(value)) { + PromisePrototypeThen( + value, + onPullResolve, + (error) => readableStreamDefaultControllerError(controller, error)); + } else { + onPullResolve(); + } + } catch (error) { + return readableStreamDefaultControllerError(controller, error); + } } function readableStreamDefaultControllerClearAlgorithms(controller) {
benchmark:
confidence improvement accuracy (*) (**) (***) webstreams/readable-async-iterator.js n=100000 *** 7.47 % ±1.31% ±1.74% ±2.27%Reacted by Debadree Chatterjee, Vinicius Lourenço, Carlos Fuentes, Yagiz Nizipli and ignoramousDoes Deno and Bun implement webstreams in their native languages or in javascript?
Removing primordials like
ArrayPrototypeShiftorArrayPrototypePushfrom the hot path led to a slight perf boost but not sure thats a good thing to doHow is the performance difference? @debadree25
I remembered seeing around 6-7% but have to check again
Does Deno and Bun implement webstreams in their native languages or in javascript?
Will investigate
Deno implements it in javascript, https://github.com/denoland/deno/blob/fc6ba92024d76d44349c36dcedd13994116db45b/ext/web/06_streams.js#L5066; they do use native functions for detaching arraybuffers/checking if an arraybuffer is detached.
Bun I assume is using webkit's streams implementation, which would be native.
Deno isn't using any array methods in its implementation,
which probably means it could be removed on node's side. Then there'd be no overhead of using ArrayPrototype primordials. Would need to look into that though 🤔.So they do eventually push it to an array, but only when the queue is empty (see: https://github.com/denoland/deno/blob/fc6ba92024d76d44349c36dcedd13994116db45b/ext/web/06_streams.js#L5648)
I've done some exploring on creating
ReadableStreamand from what I've seen, the main reason why creation is slow is because of makeTransferable, being more specific, it is expensive to create a newJSTransferable.The
Reflect.constructoperation is fast enough, but I think the limitation is inJSTransferable.Therefore, to optimize the creation of
Readable,WritableandTransform, we should take a look at how to optimize the creation ofJSTransferable.Since I don't know much about C++, I'll stop now and take a look at
.read()to see what I can find.Reacted by Debadree Chatterjee@H4ad For more info about
makeTransferablesee: nodejs/undici#1203 (comment)Reacted by Vinicius Lourençore:
makeTransferable, there's a pr up: nodejs/node#47956 (from: nodejs/undici#1203 (comment))Reacted by Vinicius LourençoSo it turns out
Reflect.constructis pretty slowRefs:
Reacted by Vinicius Lourenço and Debadree Chatterjee- added a commit that references this issue
on Sep 12, 2023 is there reflect construct anywhere in the path of read()? Today i tried a few experiments with this
ensureIsPromiseis noted to make things slow so tried modifying ensureIsPromise to not use primordials but that led to very negligible increase- Tried removing array protoype shift from the path but even that not much of a improvement
- added a commit that references this issue
on Sep 19, 2023 - added a commit that references this issue
on Sep 28, 2023 Hi @debadree25, is there a PR we can check? I might be able to spend some time soon; just have a few pending on other projects but happy to support 🙂
Dont really have any specific PR here mostly explored stuff locally @metcoder95
Maybe we can open one and start from there; we can iterate and see what do we found. Also we might get attention from more people over the reviews 👍
Reacted by Debadree Chatterjee- added a commit that references this issue
on Oct 23, 2023 Maybe we can open one and start from there; we can iterate and see what do we found. Also we might get attention from more people over the reviews 👍
made a small attempt in nodejs/node#50340
Looking at the CPU profile of this simple script:
const rs = new ReadableStream({ pull: function(controller) { controller.enqueue('a'); }, }); const reader = rs.getReader(); let x = null; const start = Date.now(); for (let i = 0; i < 1e6; i++) { const { value } = await reader.read(); x = value; } console.log(Date.now() - start); console.assert(x);
Shows something like this:
ensureIsPromise is an interesting one need to find ways to improving it without breaking wpts.
Reacted by ignoramousoptimizations here can be tricky without impacting the observable spec-defined behavior of the streams. I recommend proceeding with caution. Also, while it is possible to implement this at the c++ level it is incredibly complicated to do so correctly given how difficult it is to work with V8's C++ level Promise API.
I've documented some thoughts on general high-level optimizations that may be possible here: #134
Reacted by ignoramous and Mert Can Altin
The performance of ReadableStream.read() seems to be lacking behind other runtimes and probably can be improved
Ref: anonrig/node-benchmarks#3 (comment)