Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ovflowd
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1238 +/- ##
==========================================
+ Coverage 94.18% 94.28% +0.10%
==========================================
Files 291 293 +2
Lines 29214 29789 +575
Branches 2969 3039 +70
==========================================
+ Hits 27515 28088 +573
- Misses 1677 1679 +2
Partials 22 22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 Deploying Preview to Cloudflare 🚀Preview Deployments by commit
View all previews: View all previews ↗ |
ovflowd
marked this pull request as ready for review
November 3, 2026 20:39
ovflowd
force-pushed
the
perf/stream-cache
branch
from
November 4, 2026 18:24
dc17197 to
557f319
Compare
4 tasks done
kgeminicdev
reviewed
Nov 6, 2026
kgeminicdev
left a comment
kgeminicdev
left a comment
There was a problem hiding this comment.
Read through createReplay and the cache changes. The check-then-wait in readFrom is race-free since nothing can push between the chunks.length check and recorded.wait(), forget() at release time is safe because the reader is registered in read() rather than on first next(), and an early break cleans up through the finally.
One edge: a reader from read() that's never iterated stays in readers at index 0. .return() on an unstarted async generator skips the body, so the finally never runs. A streamsInput generator that throws during setup or returns without touching its input would then pin every chunk until recording ends, for all the other readers too. Probably fine in practice, since a failure takes the run down anyway, but worth either a line in the streamsInput docs ("iterate the input or break out of it") or a test that pins down the behavior.
ovflowd
force-pushed
the
perf/stream-cache
branch
from
November 6, 2026 13:37
557f319 to
449050c
Compare
2 of 4 tasks
ovflowd
force-pushed
the
perf/stream-cache
branch
from
November 6, 2026 19:30
449050c to
d43c259
Compare
2 of 4 tasks
ovflowd
force-pushed
the
perf/stream-cache
branch
from
November 6, 2026 19:43
d43c259 to
3ff6f70
Compare
avivkeller
force-pushed
the
perf/stream-cache
branch
from
November 9, 2026 02:26
3ff6f70 to
84d0847
Compare
Contributor
|
Every dependency edge was a full barrier: the cache collected all of a generator's chunks before its dependents started, so each stage of a build waited for the whole of the one before it. A generator can now set streamsInput: true to receive its dependency's output as an async iterable of chunks, read as they are produced. With one thread nothing runs alongside, so it still gets one array, and a dependency that doesn't stream arrives as a single chunk. Nothing sets it yet. The cache runs a streamed generator once, recording its chunks so any number of readers each read all of them, from the first, while it still runs. Once every reader has started, it lets go of a chunk as soon as all of them have read it, rather than holding the output until the last one is done. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com> Assisted-by: Claude Sonnet 5.5 <noreply@anthropic.com>
worker.stream no longer waits on a signal, so createSignal comes with the replay, which wakes its readers with it as chunks are recorded. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
From the review: the Streaming Input section, the changeset and the streamsInput and worker.stream comments say the same in fewer words. The section's example imports the processChunk it lists, and the changeset still says the cache lets a chunk go once every reader has read it. Co-authored-by: Aviv Keller <me@aviv.sh> Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
ovflowd
force-pushed
the
perf/stream-cache
branch
from
November 9, 2026 10:37
2d41aa7 to
64ccaf2
Compare
From the review, which asked for a smaller replay. Each recorded chunk resolves a promise holding it and the next one, and a reader only holds its place in that list, so a chunk every reader has passed is unreachable and collected: no index bookkeeping, no set of readers and no signal. It still records eagerly, lets chunks go once no more readers can start, and rethrows a failure after the chunks before it. replay.mjs goes from 79 lines of code to 43, and createSignal isn't needed here anymore. A new test pins what the replay is for: two readers keep up with a generator that's still recording, and the chunks both have read are collected before it goes on. A replay that keeps every chunk failed no test before. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
The generators comment said the cache logs each chunk of a streamed result as it records it, which it doesn't: a streaming generator returns before it has produced anything, so it just isn't complete there. release's JSDoc takes the review's shorter wording, and a comment restating the once() call goes. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
avivkeller
reviewed
Nov 10, 2026
Member
There was a problem hiding this comment.
Can this be inlined, it seems overly complex, right? What is a "replay" system?
Member
Author
There was a problem hiding this comment.
Basically a tee for async generators: runs once, every dependent reads all chunks. Renamed it to tee.mjs. I rather keep it separate for its tests tho :)
avivkeller
requested changes
Nov 10, 2026
avivkeller
left a comment
avivkeller
left a comment
Member
There was a problem hiding this comment.
I have some concerns with how large this PR is:
- There’s a huge difference to the cache testing file, so I assume that a lot of those tests are duplicated in the replay testing file
- Is the replay system really the best approach. It feels like we are adding another layer to our already complex caching system
From the review, which asked what a "replay" is. It splits one async generator into any number of readers, like ReadableStream's tee(): an async generator can only be iterated once, while several generators read the same dependency. createReplay becomes createTee, in tee.mjs, and its doc says what it's for. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
From the review, which found the cache's tests overlapping the tee's. Three of them only checked the tee through the cache: a reader that starts late, a failure after the chunks before it, and chunks let go after the generator ended. They go, and the tee's tests take what only they checked: the last chunk is let go once its readers are done, and a failure nothing reads isn't reported as unhandled. The cache's tests now cover how the cache uses the tee. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
Fair on the tests, a few cache ones re-tested the tee, dropped those. Not an extra layer tho, it's what runs each generator once, |
avivkeller
approved these changes
Nov 10, 2026
From the review, which found the PR's tests too large. Three cache tests checked what others already do: streaming before the generator ends is checked end to end by runGenerators' streaming test, a failure before the stream is read by runGenerators' failure test, and a stream counting as a read by the eviction test. Recording before the stream is read becomes one line of the eviction test. The test of a chunk being let go once every reader has read it takes two readers instead of three, and keeps the generator running while they read, so it now fails when eviction doesn't forget the tee's first chunk, which no test caught. runGenerators' streaming test drops its timeout race and its collecting reader, as the cache tests check one run for both kinds of readers, and the reader of a failing source drops a wait it never reached. The GC helpers both core test files use move to a shared fixture, and Array.fromAsync replaces collectChunks. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
Recording was the replay's word. The tee runs its generator and buffers its chunks for its readers. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR lets the cache stream a generator's output to the generators that read it, instead of collecting every chunk first. It's the second part of a stack that splits #1205.
Every dependency edge was a full barrier:
cache.consumecollected all of a generator's chunks before its dependent started, so each stage of a build waited for the whole of the one before it.streamsInput: trueto get its dependency's output as an async iterable of chunks, read as they're produced. With one thread nothing runs alongside, so it still gets one array, and a dependency that doesn't stream arrives as a single chunk. No generator sets it yet.createTee, which splits it into any number of readers likeReadableStream.prototype.tee(): it buffers the chunks in a linked list as they come, and each reader reads all of them, from the first, while the generator still runs. Once every reader has started, a chunk is let go as soon as all of them have read it, rather than when the last one is done.docs/creating-generators.mdhas a new Streaming Input section.Why it's built this way
What's the tee? An async generator can only be iterated once, but a generator's output can have several readers: across the stack,
metadata's is read bysection-pages,json,orama-dbandllms-txt.createTeeintee.mjsruns the generator once on the main thread, buffers its chunks as they come, and gives each reader its own position from chunk 0. It isn't a layer on top of the cache. It takes over frommain'scollect, which ran the generator once into one shared array:consumenow collects one of the tee's readers, andstreamhands one out. Earlier revisions of this PR called it the replay.Why opt in with
streamsInput? Streaming changes whatinputis, and plenty of generators need all of it before they can do anything. With the opt-in, every existing generator gets the same collected array it gets today, and so does anyone's custom one. Across the whole stack, five generators opt in:metadata,section-pages,jsx-ast,htmlandhtml-all.Why run a streamed generator once for all its readers?
mainalready runs a generator once and shares its collected array with every dependent. The tee keeps that when some of those readers stream, so no generator runs twice.Why run eagerly instead of pulling, like
share()in 9d78f0d?share()keeps a promise for every step in an array that each reader holds, and nothing clears it. So even a single streaming reader keeps its whole input alive until it's done. I measured both on the realcreateCache, with 40 chunks of 4MB and a producer at 2ms per chunk (with the index version of the tee; the linked list lets go of the same chunks):share()(153MB).Pulling also doesn't start the upstream until the reader's first
next(), and that only comes after the reader has loaded its Markdown plugins and its worker. With 100ms of setup, the producer finished at 142ms with the tee and at 232ms withshare(). The cost of running eagerly: a reader slower than its producer leaves chunks buffered (25 of 40 at peak with a 5ms reader), and the buffer shrinks again as it catches up.When does a chunk go? Only the link before a chunk holds it. Until every expected reader has asked for the result, the cache keeps the first link, so a reader that starts late still reads from chunk 0. After that,
forget()drops it and each chunk goes once the slowest reader has passed it. A reader that collects keeps everything anyway, because that's what collecting does.Why a linked list? It's Aviv's idea from 9d78f0d, a promise per step, but run eagerly and with chunks let go. It took the tee from 79 to 43 lines of code: no reader indexes to trim, and no signal to wake readers. It also fixes the edge kgeminicdev found: in the index version, a reader that was created but never iterated stayed at index 0, so no chunk could go until the generator ended. Now such a reader only keeps chunks alive while it is reachable itself.
Why not inline the tee into
caching.mjs? It's 43 lines of code, and as its own module it gets its own tests: the ones that check which chunks get garbage collected need nothing of the cache, and the cache's tests only cover how the cache uses it. Most of this PR is tests: 438 of its 703 added lines.Why can the input be read only once? An async generator returns itself from
Symbol.asyncIterator, so a secondfor awaitover the same input silently gets no chunks. Withshare()the two reads got 40 chunks, then 0. The chunks this reader already read can't be given to it again, so a second read throws instead.Why does one thread still get the whole input? Streaming has nothing to overlap with there. It would only interleave the stages and keep each stage's data alive at the same time. It's also why chore(benchmarks): codspeed #1253 can't see this stack: with
threads: 1streaming is off, and CodSpeed's simulation mode counts instructions, not wall time, so it couldn't see overlap anyway. With-p 1under--max-old-space-size=512, the stack up to refactor: move all.html into a Node.js generator #1242 passed all 11 site builds at ~1000MB peak.mainhit V8's out-of-memory abort in 1 of 8 runs and the commit before perf: let worker.stream take chunks as they arrive #1237 in 3 of 5, so passing still depends on GC timing, as perf: keep memory use down at low thread counts #1179 found.Why is this PR flat on time? No generator sets
streamsInputyet. The only change that runs is collected outputs now going through the tee, and the numbers below show nothing measurable on a full build. The gains come once perf: let metadata and section-pages start on their dependency's first chunk #1240 and perf: let jsx-ast and html start on their dependency's first chunk #1241 opt in: with perf: let jsx-ast and html start on their dependency's first chunk #1241, the site build is 17% faster thanmainat 4 threads and 20% at 6, for +26% and +24% peak memory.Benchmarks
Full Node.js docs on Node 26, against
main(which has #1237). Each number is the median of 5 interleaved runs (4 for both builds of this branch at 6 threads, and formain's legacy build at 6). Peak memory is RSS summed over the whole process tree. Site is-t orama-db -t json -t llms-txt -t section-pages; legacy is-t legacy-html-all -t legacy-json-all.mainAll of this is within the noise between runs.
main's own peaks ranged from 1505 to 1583MB on the legacy build at 4 threads, from 1932 to 2057MB on legacy at 6, and from 2304 to 2371MB on the site build at 6.The linked list against the old index version, measured on #1241 (before its manifest fix) where something streams, everything else equal:
At 6 threads a second round of 7 runs gave the linked list 6.91s, 2758MB against 7.00s, 2734MB, so no change there either.
Validation
Added tests for
createTee(chunks every reader has read let go while it still runs, and the last one once they're done, every reader gets every chunk in order, a chunk let go before a reader read it, a failure, and no unhandled rejection when nothing reads it), for the cache (one run for streaming and collecting readers, a non-streaming result as one chunk, running a stream before it's read and counting it as a read, a stream read only once, and chunks let go once every reader has read them while the generator still runs), with the garbage collection helpers both test files use in a shared fixture, and forrunGenerators(streaming to a generator that streams its input while its source still runs, a failing dependency read as a stream, and giving it the whole input with one thread). Built on 12 of Node.js's API docs at 1 and 4 threads, the output is byte-identical tomain's.Related Issues
Refs: #1205
Check List
node --run testand all tests passed.node --run format:check&node --run lint.