Improve Node.js performance using an AsyncIterable #9614
Conversation
🦋 Changeset detectedLatest commit: 2826812 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
!bench render |
|
!bench render |
PR BenchmarkRender
Main BenchmarkRender
|
|
FWIW I don’t expect With the #9603 preview release the benefit kicked in once list items in one 
That benchmark also suggests a similar slight slowdown for smaller list sizes seen in the benchmark above:
Not sure if those are statistically significant though — just 10 runs each using Would love to run this same benchmark on a preview release of this PR when it’s ready! |
|
@delucis I'm going to update the benchmark (or create a new one, not sure), which shows the benefit. |
Co-authored-by: Florian Lefebvre <[email protected]>
.changeset/hip-cherries-behave.md
Outdated
|
|
||
| Improves Node.js streaming performance | ||
|
|
||
| This uses an AsyncIterable instead of a ReadableStream to do streaming in Node.js. This is a non-standard enhancement by Node, so this is done only in that environment. |
There was a problem hiding this comment.
What's the implication of this being "a non-standard enhancement by Node" and why does the reader care about this?
"Non-standard" doesn't always imply something positive, (is this a hack? are we jumping on to something experimental?) and in fact might make people worry/wonder. So this feels maybe a little too jargon-y when I'm assuming that this could actually be framed as something cool and helpful!
Just a thought!
There was a problem hiding this comment.
It just means that it won't be run non-Node environments, for ex. in Cloudflare. Those will take the slower path.
Yeah it can be tricky, I'm not always sure who the target of these changesets are. A lot of changes are implementation details that most won't understand or care about, but some will. And they can be useful for understanding the change on the part of maintainers. But we don't usually do that.
Thinking... do you have a suggested way to rephrase this?
Co-authored-by: Emanuele Stoppa <[email protected]>
Co-authored-by: Emanuele Stoppa <[email protected]>
|
!preview render-nodejs |
|
Snapshots have been released for the following packages:
Publish LogBuild Log |
|
Ran the same benchmark as mentioned in #9614 (comment) using the prerelease and saw a similar improvement to benchmarks for #9603: build time roughly doubles from 5s to 10s as sidebar size increases from 0 to 7,000 items. With the public release, build time increased c. 20x for the same sidebar size. 
|
| } | ||
|
|
||
| // Create a new array with total length and merge all source arrays. | ||
| let mergedArray = new Uint8Array(length); |
There was a problem hiding this comment.
Did you consider using Buffer.concat? In Bun, we've optimized Buffer.concat a lot, it should be meaningfully faster than this approach.
If for some reason Buffer.concat cannot be used, Buffer.allocUnsafe should also be faster here - there's no risk of uninitialized memory leaking since the full length is counted and will not change
There was a problem hiding this comment.
No I did not know that it would be faster. Would be happy to take a PR on this, we have benchmarks we can compare.
| }; | ||
|
|
||
| const destination: RenderDestination = { | ||
| write(chunk) { |
There was a problem hiding this comment.
Would you be open to a PR that makes this fast in Bun as well?
It would use [Symbol.asyncIterator] and you can yield either ArrayBufferView or string literals, so you can skip the encode/decode step (if I'm reading this code correctly). This would make faster and use a lot less memory.
Then the iterator function would essentially be:
// earlier
let { promise, resolve, reject } = Promise.withResolvers();
let pendingChunks = [];
// inside the iterator
async function* iterator() {
while (true) {
await promise;
({ promise, resolve, reject } = Promise.withResolvers());
const chunks = pendingChunks;
pendingChunks = [];
yield* chunks;
if (chunks.length === 0) return;
}
}
const destination: RenderDestination = {
write(chunk) {
// ....There was a problem hiding this comment.
Thanks, assuming this would need to conditionally run only in Bun we'd like to hold off on that for now. I didn't particularly want to do the change in this PR but the difference was too big to ignore.
There was a problem hiding this comment.
I've not looked into performance, but seems you could even return an async generator of strings directly. I don't see why you would need to construct the async iterator yourself, and why you need to mess around with that buffer at all, but perhaps I'm missing something? Returning the generator would be a lot less code, and the following works in Node.js:
async function* renderToAsyncIterable() {
yield await Promise.resolve('a');
yield await Promise.resolve('b');
yield await Promise.resolve('c');
}
const res = new Response(renderToAsyncIterable())
await res.text()Note that you can also use for await...of to iterate over async iterables when needed.
There was a problem hiding this comment.
In my past refactors, I found that generators tend to be slow. So I think we should avoid it even if it may be easier to read.
There was a problem hiding this comment.
@bluwy right... seems like generators are 1.8-2x slower than doing it manually (at least in node.js), but seems that they scale equally. But that still leaves the question why not to return a AsyncIterable<string | Uint8Array> and avoid creating another buffer.
Changes
Testing
Docs
N/A, bug fix