stream: speed up async iteration of Readable#64447
Conversation
|
Review requested:
|
Replace the async generator backing Symbol.asyncIterator with a hand-rolled iterator. The generator machinery costs several extra promise allocations and microtask hops per chunk: yield awaits the yielded value and resolves the pending request through separate promises. Buffered chunks are now delivered as an already-resolved promise, one microtask sooner than before. Thenable chunks are still awaited before delivery, requests received while a next() is outstanding are queued, and return()/throw() before the first next() complete the iterator without touching the stream. The earlier delivery is observable by code racing an abort against the first chunk. The flatMap AbortSignal test relied on such a race; it is reworked to abort deterministically while two mappers are in flight, asserting the concurrency limit, in-flight cancellation and rejection, without depending on delivery timing or timers. streams/readable-async-iterator.js sync='yes': +32.59% (***) streams/readable-async-iterator.js sync='no': +9.84% (***) Assisted-by: Claude Fable 5 Signed-off-by: Matteo Collina <matteo.collina@gmail.com>
2307152 to
6f9a2f3
Compare
| if (typeof chunk.then === 'function') { | ||
| PromisePrototypeThen(PromiseResolve(chunk), (value) => { | ||
| inFlight = false; | ||
| resolve({ done: false, value }); | ||
| if (queue !== null) drain(); | ||
| }, (err) => { | ||
| inFlight = false; | ||
| settleError(err, reject); | ||
| if (queue !== null) drain(); | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If then is a getter that e.g. throws on the second access, this code might throw when previously it wouldn't have. We can protect against that by storing the initial value we're getting (that might also avoid an additional promise allocation).
| if (typeof chunk.then === 'function') { | |
| PromisePrototypeThen(PromiseResolve(chunk), (value) => { | |
| inFlight = false; | |
| resolve({ done: false, value }); | |
| if (queue !== null) drain(); | |
| }, (err) => { | |
| inFlight = false; | |
| settleError(err, reject); | |
| if (queue !== null) drain(); | |
| }); | |
| return; | |
| } | |
| const { then } = chunk; | |
| if (typeof then === 'function') { | |
| FunctionPrototypeCall(then, chunk, (value) => { | |
| inFlight = false; | |
| resolve({ done: false, value }); | |
| if (queue !== null) drain(); | |
| }, (err) => { | |
| inFlight = false; | |
| settleError(err, reject); | |
| if (queue !== null) drain(); | |
| }); | |
| return; | |
| } |
| if (typeof chunk.then === 'function') { | ||
| inFlight = true; | ||
| return PromisePrototypeThen( | ||
| PromiseResolve(chunk), onChunkFulfilled, onChunkRejected); |
There was a problem hiding this comment.
Same here
| if (typeof chunk.then === 'function') { | |
| inFlight = true; | |
| return PromisePrototypeThen( | |
| PromiseResolve(chunk), onChunkFulfilled, onChunkRejected); | |
| const { then } = chunk; | |
| if (typeof then === 'function') { | |
| inFlight = true; | |
| return FunctionPrototypeCall(then, chunk, onChunkFulfilled, onChunkRejected); |
| settleError(err, reject); | ||
| } | ||
|
|
||
| return { |
There was a problem hiding this comment.
This object should ideally have a prototype of AsyncIteratorPrototype.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64447 +/- ##
==========================================
- Coverage 90.24% 90.23% -0.02%
==========================================
Files 741 741
Lines 241384 241544 +160
Branches 45480 45527 +47
==========================================
+ Hits 217844 217954 +110
- Misses 15097 15149 +52
+ Partials 8443 8441 -2
🚀 New features to boost your workflow:
|
| return new Promise((resolve, reject) => { | ||
| if (inFlight) { | ||
| queue ??= []; | ||
| queue.push({ type: 'return', value, resolve, reject }); |
There was a problem hiding this comment.
| queue.push({ type: 'return', value, resolve, reject }); | |
| queue.push({ __proto__: null, type: 'return', value, resolve, reject }); |
| return new Promise((resolve, reject) => { | ||
| if (inFlight) { | ||
| queue ??= []; | ||
| queue.push({ type: 'next', value: undefined, resolve, reject }); |
There was a problem hiding this comment.
| queue.push({ type: 'next', value: undefined, resolve, reject }); | |
| queue.push({ __proto__: null, type: 'next', value: undefined, resolve, reject }); |
| return new Promise((resolve, reject) => { | ||
| if (inFlight) { | ||
| queue ??= []; | ||
| queue.push({ type: 'throw', value: err, resolve, reject }); |
There was a problem hiding this comment.
| queue.push({ type: 'throw', value: err, resolve, reject }); | |
| queue.push({ __proto__: null, type: 'throw', value: err, resolve, reject }); |
|
|
||
| function drain() { | ||
| while (!inFlight && queue.length > 0) { | ||
| const req = queue.shift(); |
There was a problem hiding this comment.
Can we replace queue.shift(); with an index-based queue for high throughput?
| } | ||
| return new Promise((resolve, reject) => { | ||
| if (inFlight) { | ||
| queue ??= []; |
There was a problem hiding this comment.
We should use FixedQueue here since a normal Array shift inside a while loop to empty it is O(n2):
node/lib/internal/fixed_queue.js
Line 91 in 17163ea
| while (!inFlight && queue.length > 0) { | ||
| const req = queue.shift(); | ||
| if (req.type === 'next') { | ||
| processNext(req.resolve, req.reject); | ||
| } else if (req.type === 'return') { | ||
| processReturn(req.value, req.resolve); | ||
| } else { | ||
| processThrow(req.value, req.reject); | ||
| } | ||
| } |
There was a problem hiding this comment.
We need a guard here I think because drain() is recursive when chunks are already buffered
Each queued request adds another nested drain -> processNext -> pump cycle
| while (!inFlight && queue.length > 0) { | |
| const req = queue.shift(); | |
| if (req.type === 'next') { | |
| processNext(req.resolve, req.reject); | |
| } else if (req.type === 'return') { | |
| processReturn(req.value, req.resolve); | |
| } else { | |
| processThrow(req.value, req.reject); | |
| } | |
| } | |
| if (draining) { | |
| return; | |
| } | |
| draining = true; | |
| try { | |
| while (!inFlight && queue.length > 0) { | |
| const req = queue.shift(); | |
| if (req.type === 'next') { | |
| processNext(req.resolve, req.reject); | |
| } else if (req.type === 'return') { | |
| processReturn(req.value, req.resolve); | |
| } else { | |
| processThrow(req.value, req.reject); | |
| } | |
| } | |
| } finally { | |
| draining = false; | |
| } |
There was a problem hiding this comment.
Currently, this script throws a stack error with this patch:
'use strict';
const assert = require('node:assert');
const { Readable } = require('node:stream');
const count = 20_000;
const stream = new Readable({
objectMode: true,
read() {},
});
const iterator = stream[Symbol.asyncIterator]();
const requests = [];
for (let i = 0; i < count; i++) {
requests.push(iterator.next());
}
for (let i = 0; i < count; i++) {
stream.push(i);
}
stream.push(null);
Promise.all(requests).then((results) => {
assert.strictEqual(results.length, count);
for (let i = 0; i < count; i++) {
assert.deepStrictEqual(results[i], {
done: false,
value: i,
});
}
console.log('All requests completed without recursion');
});
// RangeError: Maximum call stack size exceeded
Replace the async generator backing
Readable.prototype[Symbol.asyncIterator](and.iterator()) with a hand-rolled iterator.The generator machinery costs several extra promise allocations and microtask hops per chunk:
yieldawaits the yielded value and resolves the pending request through separate promises, and everynext()goes through the async generator request queue. The hand-rolled iterator delivers buffered chunks as an already-resolved promise.The observable semantics are preserved:
next()/return()/throw()calls received while a request is outstanding are queued and processed in order — includingreturn()while waiting for data, which still completes only once the pending read settles;return()/throw()before the firstnext()complete the iterator without attaching listeners or destroying the stream;finallyteardown logic (destroyOnReturn,autoDestroy, half-open duplex preservation) is unchanged;aggregateTwoErrorsis unchanged.The one observable difference is that buffered chunks are delivered one microtask sooner than before, since the generator's
yieldperformed an implicitAwaiton the yielded value. This is visible to code racing an abort against the first chunk. The flatMapAbortSignaltest relied on such a race (aqueueMicrotask'd abort beating the first chunk); it is reworked to be deterministic and timer-free: two mappers block until their signal aborts, the abort fires while both are in flight, and the test asserts the concurrency limit is respected (exactly two mappers start), in-flight mappers are cancelled through their signal, and iteration rejects withAbortError. The reworked test passes against both the old and the new implementation.New regression tests cover the subtler iterator behaviors (thenable unwrapping, rejected thenables,
throw(), pre-startthrow(), concurrentnext()ordering, andreturn()queued behind a pendingnext()); they also pass against both implementations.Benchmark (
benchmark/compare.js, 30 runs):No changes on
pipe.js/readable-readall.js.🤖 Generated with Claude Code