Performance of WHATWG ReadableStream.read()

[debadree25]

The performance of ReadableStream.read() seems to be lacking behind other runtimes and probably can be improved

Ref: https://github.com/anonrig/node-benchmarks/pull/3#issuecomment-1546646451

[debadree25]

This could also probably impact fetch perf too!

[debadree25]

Removing primordials like ArrayPrototypeShift or ArrayPrototypePush from the hot path led to a slight perf boost but not sure thats a good thing to do

[KhafraDev]

Getting rid of ensureIsPromise improved perf by ~7.5% locally, but broke at least 2 tests from the WPTs. Maybe someone else will feel inspired:

```diff 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%

[anonrig]

Does Deno and Bun implement webstreams in their native languages or in javascript?

[anonrig]

Removing primordials like ArrayPrototypeShift or ArrayPrototypePush from the hot path led to a slight perf boost but not sure thats a good thing to do

How is the performance difference? @debadree25

[debadree25]

I remembered seeing around 6-7% but have to check again

[debadree25]

Does Deno and Bun implement webstreams in their native languages or in javascript?

Will investigate

[KhafraDev]

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)

[H4ad]

I've done some exploring on creating ReadableStream and 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 new JSTransferable.

The Reflect.construct operation is fast enough, but I think the limitation is in JSTransferable.

Therefore, to optimize the creation of Readable, Writable and Transform, we should take a look at how to optimize the creation of JSTransferable.

Since I don't know much about C++, I'll stop now and take a look at .read() to see what I can find.

[RafaelGSS]

@H4ad For more info about makeTransferable see: https://github.com/nodejs/undici/issues/1203#issuecomment-1100969210

[ignoramous]

re: makeTransferable, there's a pr up: https://github.com/nodejs/node/pull/47956 (from: https://github.com/nodejs/undici/issues/1203#issuecomment-1621256156)

[rluvaton]

So it turns out Reflect.construct is pretty slow

Refs:

• https://github.com/nodejs/node/pull/49089

[debadree25]

is there reflect construct anywhere in the path of read()? Today i tried a few experiments with this

1) ensureIsPromise is noted to make things slow so tried modifying ensureIsPromise to not use primordials but that led to very negligible increase 2) Tried removing array protoype shift from the path but even that not much of a improvement

[metcoder95]

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 🙂

[debadree25]

Dont really have any specific PR here mostly explored stuff locally @metcoder95

[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 👍

[debadree25]

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 https://github.com/nodejs/node/pull/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.

[jasnell]

optimizations 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: https://github.com/nodejs/performance/issues/134