WHATWG Streams

[ronag]

WHATWG Streams are slow, and one of the primary reasons for slow performance with fetch in Node.

[mcollina]

Every stream creates an Error, and creating errors is slow in Node.js because of how we construct them. We also override the base types; I think we should not do that.

[RafaelGSS]

Further info: nodejs/undici#1203 (comment)

Every stream creates a NodeError (even in the happy path) which is slower than the common Error: https://github.com/RafaelGSS/nodejs-bench-operations/blob/main/RESULTS-v18.md#nodejs-error

[anonrig]

I'm going to work on this in the next couple of weeks.

@RafaelGSS Can you share where stream creates a NodeError? I couldn't find the path.
@mcollina Can you share the reference for override the base types?

[anonrig]

I did a small perf change to ReadableStream (nodejs/node#45489)

[ronag]

I did a small perf change to ReadableStream (nodejs/node#45489)

I think you are confusing Readable and ReadableStream. That PR has nothing to do with WHATWG streams as far as I can see.

[metcoder95]

Please correct me if I'm wrong but are @RafaelGSS referring to these errors https://github.com/nodejs/node/blob/1bbd14eac20684a733250629c7896262c0ca4576/lib/internal/webstreams/readablestream.js#L33-L38?

An example from RedeableStream loads this set of errors, which are pre-constructed in https://github.com/nodejs/node/blob/1bbd14eac20684a733250629c7896262c0ca4576/lib/internal/errors.js.

If these are the errors, these should be somehow lazy-loaded or replaced by the common Error constructor?

[RafaelGSS]

No. Actually, per spec when a default writable/readable is released an error should be propagated https://streams.spec.whatwg.org/#writable-stream-default-writer-release, and nowadays this is quite expensive. My initial analysis expands on this.

[RafaelGSS]

Honestly, even changing to common Errors, we won't see much improvement.

[mcollina]

Have you tried with the primoridial Error? https://github.com/nodejs/node/blob/1bbd14eac20684a733250629c7896262c0ca4576/lib/internal/errors.js#L27

[RafaelGSS]

Yes, it improves, but it's not a significant improvement that would make sense to break the spec. The problem isn't the kind of Error utilized, it's more about propagating an error (or rejecting a promise) even in the happy path -- which is quite expensive.

I'm tempted to consider a WebStreams implementation on C++, but this is just a guess.

[mcollina]

The problem is not breaking the spec. We override all the Errors in node.. even the ones that we shouldn't.

[metcoder95]

Noob question, but the primordials already contain the TypeError class, in theory, the spec specifies to just construct the TypeError and keep it until rejection if further validation fails.

Can be used bypassing the call to https://github.com/nodejs/node/blob/1bbd14eac20684a733250629c7896262c0ca4576/lib/internal/errors.js#L360 and having at least no performance regression meanwhile respecting the spec, it or will have the same impact?
And subsequently, does the error has to have all the properties that the previous method attaches to it?

[RafaelGSS]

Guys, It's on my radar. I'll be working on it when I have time.

Just created a benchmark PR nodejs/node#45876.

[metcoder95]

Are you planning to have some specific action items once the assessment? I’m more than happy to help you with if possible 🙂

[RafaelGSS]

Are you planning to have some specific action items once the assessment? I’m more than happy to help you with if possible 🙂

I'll try to collect some insights and post them here. Eventually, it can be a list of actionable

[RafaelGSS]

So, I've spent some time on it again, and here's the summary:

• I'm using a local branch to perform those tests: https://github.com/RafaelGSS/node/tree/webstream-investigation
• I'm using undici.fetch as a real use case of web streams, the benchmark files are available at https://github.com/RafaelGSS/nodejs-webstreams-perf/blob/main/bench/fetch.js
• I did all the tests on my developer machine and on a dedicated server, and the results seem consistent.
• We can achieve a 30% of improvement in the Promise.reject + NodeError improvement. The following change bumps the performance by 30% RafaelGSS/node@ecd6065
• For each fetch call, a TransformStream is created (which per se, creates a WritableStream and a ReadableStream) and all those are transferable objects.
• As previously mentioned in fetch() performance parity undici#1203 (comment) and Userland transferable/cloneable objects node#37080. makeTransferable is terrible for performance and after removing this call, we are able to achieve more 30% of improvement. See RafaelGSS/node@e5e707b.

I'll keep investigating other clear wins when I have time, but these two are great starting points.

[KhafraDev]

ReadableStreamDefaultReader.releaseLock performance can be improved by only creating errors as needed:

diff

diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js
index 5344785b90..18c86163c5 100644
--- a/lib/internal/webstreams/readablestream.js
+++ b/lib/internal/webstreams/readablestream.js
@@ -2029,15 +2029,22 @@ function readableStreamDefaultReaderRelease(reader) {
   readableStreamReaderGenericRelease(reader);
   readableStreamDefaultReaderErrorReadRequests(
     reader,
-    new ERR_INVALID_STATE.TypeError('Releasing reader')
+    'Releasing reader'
   );
 }

-function readableStreamDefaultReaderErrorReadRequests(reader, e) {
-  for (let n = 0; n < reader[kState].readRequests.length; ++n) {
-    reader[kState].readRequests[n][kError](e);
+function readableStreamDefaultReaderErrorReadRequests(reader, m) {
+  const { readRequests } = reader[kState];
+
+  if (readRequests.length) {
+    const error = new ERR_INVALID_STATE.TypeError(m);
+
+    for (let n = 0; n < readRequests.length; ++n) {
+      readRequests[n][kError](error);
+    }
+
+    readRequests.length = 0
   }
-  reader[kState].readRequests = [];
 }

 function readableStreamBYOBReaderRelease(reader) {

For a simple use case, this no longer creates an error:

const readable = getReadableOrSomething()

const reader = readable.getReader()
await reader.read()
reader.releaseLock()

Although I haven't benchmarked the code and haven't run any tests other than the WPTs (which all passed). I don't think releaseLock is a hot path either but it's the first thing I noticed while reading Rafael's comment above.

[RafaelGSS]

@KhafraDev I think that might be a following up pr after nodejs/node#46086, but instead of creating an error, re-use the one mentioned. But honestly, I don't think it will increase the performance because we're now re-using the errors.

[KhafraDev]

I see that is just landed! I guess my solution is a viable alternative if there is an issue with re-using errors. 😄

[rluvaton]

I have hunch it's related to a lot of promise creation (for example the ready promise on writable) node streams do not uses promises extensively IIRC