Skip to content
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

Performance of WHATWG ReadableStream.read() #82

Open
debadree25 opened this issue May 15, 2023 · 18 comments
Open

Performance of WHATWG ReadableStream.read() #82

debadree25 opened this issue May 15, 2023 · 18 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@debadree25
Copy link
Member

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

Ref: anonrig/node-benchmarks#3 (comment)

@debadree25
Copy link
Member Author

This could also probably impact fetch perf too!

@anonrig anonrig added good first issue Good for newcomers help wanted Extra attention is needed performance-agenda labels May 15, 2023
@debadree25
Copy link
Member Author

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

@benjamingr benjamingr changed the title Performance of WHATWYG ReadableStream.read() Performance of WHATWG ReadableStream.read() May 16, 2023
@KhafraDev
Copy link
Member

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 --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
Copy link
Member

anonrig commented May 29, 2023

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

@anonrig
Copy link
Member

anonrig commented May 29, 2023

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
Copy link
Member Author

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

@debadree25
Copy link
Member Author

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

Will investigate

@KhafraDev
Copy link
Member

KhafraDev commented May 29, 2023

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
Copy link
Member

H4ad commented Jun 29, 2023

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
Copy link
Member

@H4ad For more info about makeTransferable see: nodejs/undici#1203 (comment)

@ignoramous
Copy link

re: makeTransferable, there's a pr up: nodejs/node#47956 (from: nodejs/undici#1203 (comment))

@rluvaton
Copy link
Member

rluvaton commented Sep 11, 2023

So it turns out Reflect.construct is pretty slow

Refs:

@debadree25
Copy link
Member Author

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

nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Sep 19, 2023
Refs: nodejs/performance#82
PR-URL: #49622
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
ruyadorno pushed a commit to nodejs/node that referenced this issue Sep 28, 2023
Refs: nodejs/performance#82
PR-URL: #49622
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@metcoder95
Copy link
Member

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
Copy link
Member Author

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

@metcoder95
Copy link
Member

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
Copy link
Member Author

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:
Screenshot 2023-10-23 at 8 04 23 PM

ensureIsPromise is an interesting one need to find ways to improving it without breaking wpts.

alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
Refs: nodejs/performance#82
PR-URL: nodejs#49622
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@jasnell
Copy link
Member

jasnell commented Nov 12, 2023

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: #134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

9 participants