Studio: stop the streamed reply being flattened on every arrival - #9049
Conversation
PR 9012 removed two whole-buffer scans from the chat stream adapter and
left about 16 ms per side behind. That remainder is not a scan. Every
`cumulativeText += delta` leaves a cons string, and the first thing that
reads it copies the whole reply flat, so a single `charCodeAt` costs the
same as a full scan. Measured over a 220,000 character reply of 55,005
arrivals, medians of 5 to 7 repetitions, paired and interleaved:
append only, nothing reads the buffer 0.84 ms
one bounded scan of the tail per arrival 1715.98 ms
Three steps ran per arrival and all three read the buffer, so all three
paid that. Each now takes what the arrival added instead.
The think-tag tracker takes the delta rather than slicing it back out of
the buffer, and keeps the seven characters in front of it itself, which
is the most a tag split across arrivals can hide behind. 1743.92 ms to
4.19 ms at 220K.
The trailing `${...}` strip cannot avoid touching the end of the reply
when it fires, so a watch decides whether it could fire at all, from the
deltas: the last non-whitespace character, the last `${`, and the two
most recent `}`. It never says no when the strip would cut, so nothing
that used to be stripped survives, and a reply that never ends in a
brace never wakes the strip. 1715.98 ms to 6.74 ms at 220K.
`parseAssistantContent` over the whole reply, which `liveAssistantContent`
runs on every arrival, becomes an incremental parse that keeps the parts
it has already produced and extends them with the delta. It holds back
the trailing characters that could still turn out to be a tag, so `<thi`
is text until `nk>` arrives and reasoning after, matching a full reparse
at every state. Runs are cut at the tool-call cursors exactly as before,
including that think state resets at a boundary. 1978.78 ms to 7.37 ms
at 220K.
The retained state describes an append-only reply. A rewritten prefix, a
removed suffix or a tool call landing behind the end shows up as a length
or boundary mismatch and reparses from the buffer, which is what this
replaces, so those paths are no slower than before. An external
continuation whose prefix `joinContinuation` may repair never uses the
incremental path at all.
Whole per-arrival path, before to after, medians of 5:
55,000 chars 13,755 arrivals 236.89 ms -> 4.06 ms 58x
110,000 chars 27,505 arrivals 795.09 ms -> 5.73 ms 139x
220,000 chars 55,005 arrivals 4882.98 ms -> 13.06 ms 374x
400,000 chars 100,005 arrivals 17060.48 ms -> 21.57 ms 791x
The factor grows with the reply because the old cost was quadratic and
steepens again past 131,072 characters, where a flattened string stops
fitting in a regular heap object.
Tests. A differential fuzz test drives random arrival streams, with tags
split at every offset, tool boundaries, unclosed tags, empty arrivals and
truncations, and requires the incremental parse to deep-equal a full
reparse after every single arrival. The think tracker and the placeholder
watch keep their existing suites, driven through the new delta API, and
the watch gains a soundness sweep: for every state of every stream, if
the strip would cut, the watch admitted it.
Two source pins from 9012 are repointed rather than dropped. The one that
asserted the exact spelling of the tracker call now asserts what that
spelling protected: the reply grows through one call, which is what keeps
all three delta-fed pieces in step. A new pin lists the forms in which
the loop may mention the buffer at all, because character counting cannot
see this defect: a `charCodeAt` and a full scan read the same characters
and cost the same, so only a rule about touching the buffer catches it.
d56dcfe to
79a38ec
Compare
No conflict: 9085 and 9088 land in the assistant-ui render schedule and its tests, which this branch does not touch.
|
A coverage gap worth closing before this merges, plus a branch-state note. The gapThe watch lets the adapter skip the strip on almost every arrival. That is only sound while one invariant holds:
If it is ever false at such a moment, the reply keeps a
Measured, by mutating the source rather than by reading it. Setting
So the mutation is a real defect the current tests do not see. The test
It also asserts a strip actually fired, so it cannot pass by never exercising the thing it is about. Confirmed both ways: it fails on Branch stateThe commit adding it is prepared but deliberately not pushed yet, to avoid racing another rebase in flight on this branch. It applies on top of Two other results from the same round, for the record: |
|
@codex review |
|
Re-verified after the The reseed test still discriminates: with Two things found in the same round that belong outside this PR, noted here so they do not get pulled into it:
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Heads up on an adjacency, no action needed on this branch. #9101 fixes #9098 by moving the trailing So the two touch adjacent lines and will need a small merge whichever lands second. If #9101 lands first, Also worth knowing for the reseed and watch tests here: after #9101 the buffer inside the loop only ever grows. The tracker's shrink branch and the watch's |
Verification against current mainRe-verified end to end after main moved: 9085 and 9088 merged at 12:06Z, and this branch now carries The barThe rendered output must be the same as before, character for character, not merely look the same. Deferred work is fine; missing content is not. Live sampling cannot show that. Three runs of one prompt against Correctness
The differential corpus now includes carriage returns, added because 9088 made the line ending a live variable below this code. A lone Cases covered: ordinary reply, unterminated code fence, 13,655 character reply, no fences, only fences, CJK, RTL, mixed RTL with CJK and code, reasoning, one token, empty, plus interrupt, reload and two tabs. What the screenshots showTwo isolated installs, merge base 34c9d98 against this head, one scene driven against both, the same recorded reply served to each. Mid-stream, both sides part way through the same reply:
Settled:
The halves are identical by design, so the evidence is the fact list, not the picture. Read from the DOM on each side, equal on both:
Only the clocks moved: 11.9s against 12.77s to settle, 61 ms against 78 ms longest paint gap. CostBoth claims reproduce. Per-arrival bookkeeping, paired in one process:
Stated plainly: below about 10,000 characters this is at parity or marginally slower. The win is on long replies. TestsMutation tested by planting 21 plausible bugs. 19 are caught. The two that survive are equivalent mutants with no observable effect: the Two of the four that survived on the first pass were real gaps, both in What was not tested
Unrelated, found while measuringStreaming a JavaScript template literal through an external provider permanently deletes text: Staging CI on Datta0/unsloth-staging-3, since the org queue is carrying 34 pending checks per PR. |
Follow-up: the interrupt case, settled properlyThe earlier note said the interrupt scenario cannot separate the two trees. Here is the full study rather than the summary, because a case that disagrees at all deserves the numbers. The scenario holds the replayed stream after an exact SSE event count, so both sides receive identical bytes, then clicks Stop. The whole disagreement is one 8 character token, Each tree compared against itself disagrees more often than the two trees compared against each other, and every tree produces both values. So the split is where the Stop lands relative to the paint, not what either tree renders. The case is reported, and it is not evidence in either direction. Two other cases were re-run rather than assumed:
Staging CIBoth PRs are green on ubuntu-latest, macos-14, windows-latest, frontend ubuntu-latest, frontend macos-14 and studio-playwright. The first Playwright attempt failed with |
… reply Fixes unslothai#9098. The trailing `${...}` strip ran on every SSE arrival and assigned its result back, so "ends with ${...}" was tested against every prefix of the reply rather than the reply. The one arrival whose buffer ended at a complete fragment was cut, and the reassignment made the cut permanent: in return `Hi, ${name}!` 21 chars out return `Hi,!` 13 chars The strip now runs once, after the stream has finished. The fragment it was added for in unslothai#4706 is still removed, because that one really is at the end of a completed answer. Rebased onto unslothai#9049. The watch it added still gates the scan, and now saves the whole reply from being flattened rather than one arrival's worth, so nothing on the arrival path can flatten the buffer at all. Abort keeps the buffer whole: that tail is a prefix again, so stripping it would be the same bug. `producedReplyText` is the same case one step in, for a continuation that finishes without a text or reasoning delta and so holds nothing but the partial it was seeded with.
… reply (unslothai#9101) Fixes unslothai#9098. The trailing `${...}` strip ran on every SSE arrival and assigned its result back, so "ends with ${...}" was tested against every prefix of the reply rather than the reply. The one arrival whose buffer ended at a complete fragment was cut, and the reassignment made the cut permanent: in return `Hi, ${name}!` 21 chars out return `Hi,!` 13 chars The strip now runs once, after the stream has finished. The fragment it was added for in unslothai#4706 is still removed, because that one really is at the end of a completed answer. Rebased onto unslothai#9049. The watch it added still gates the scan, and now saves the whole reply from being flattened rather than one arrival's worth, so nothing on the arrival path can flatten the buffer at all. Abort keeps the buffer whole: that tail is a prefix again, so stripping it would be the same bug. `producedReplyText` is the same case one step in, for a continuation that finishes without a text or reasoning delta and so holds nothing but the partial it was seeded with.
1 participant
Stacked on #9012 (
perf-chat-adapter-scans). Review that one first; this branch is one commit on top of it.What is left after #9012
#9012 removed two whole-buffer scans from the chat stream adapter and reported about 16 ms per side still remaining. That remainder is not a scan, and it cannot be removed by bounding one.
Every
cumulativeText += deltaleaves a V8 cons string. The first thing that reads it copies the whole reply flat. So the amount a step inspects is irrelevant: onecharCodeAtcosts the same as a full scan. Measured over a 220,000 character reply of 55,005 arrivals of 4, medians of 7, paired and interleaved:Three things are free on the accumulating buffer: appending,
.length, and the degenerateslice(0, length)that V8 answers with the rope itself. Exactly one thing costs, at about 1000x, and it is forcing the rope flat.That the remainder is the flatten and not the reads is directly checkable. Same reads, same buffer contents, answers asserted identical, only the representation differs. 55,000 characters, 13,755 arrivals, medians of 9:
Three steps ran per arrival and all three read the buffer, so all three paid it. Each now takes what the arrival added instead.
What changed
appendCumulativebecomes the single place the reply grows, so everything derived from it sees the same characters in the same order.${...}strip cannot avoid touching the end of the reply when it fires, so a watch decides from the deltas whether it could fire at all, from the last non-whitespace character, the last${, and the two most recent}. It never says no when the strip would cut, so nothing that used to be stripped survives, and a reply that never ends in a brace never wakes the strip.parseAssistantContentover the whole reply, whichliveAssistantContentran on every arrival, becomes an incremental parse that keeps the parts it has produced and extends them with the delta. It holds back the trailing characters that could still turn out to be a tag, so<thiis text untilnk>arrives and reasoning after. Runs are cut at the tool-call cursors exactly as before, including that think state resets at a boundary.The retained state describes an append-only reply. A rewritten prefix, a removed suffix, or a tool call landing behind the end shows up as a length or boundary mismatch and reparses from the buffer, which is what this replaces, so those paths are no slower than before. An external continuation whose prefix
joinContinuationmay repair never uses the incremental path at all.Before and after
Whole per-arrival path, paired and interleaved, medians of 5, answers cross-checked identical between the two sides at every size:
The factor grows because the old cost was quadratic. It steepens again past 131,072 characters, where a flattened one-byte string stops fitting in a regular heap object and every flatten allocates in large-object space instead. Bisecting the transition, medians of 9:
v8.getHeapSpaceStatistics()confirms it:large_object_spacegrows 0.0 MB at 131,000 characters and 1.2 MB at 140,000.This is V8's
kMaxRegularHeapObjectSize, half a 256 KB page, andHeap::AllocateRawdispatches on it with a baresize_in_bytes > kMaxRegularHeapObjectSize. The cliff is a known V8 issue rather than something specific to us: v8:13085 reports a 7x discontinuity at exactly this boundary, and a V8 developer's reply there notes that the 128 KB limit "is not outlandishly big", so ordinary short-lived objects reach it. Large objects skip young space entirely, which is why the cost characteristics change so sharply. See also the write-up in danbev/learning-v8 notes/heap.md.This is the regime a long code-heavy reply actually lands in.
The after column is linear in the reply, which is the check that no flatten is left.
These are node measurements of the adapter loop. The browser harnesses do not reach this code: PR 9016's heavy-thread harness has no streaming action, and PR 8969's stream harness drives a stub
ChatModelAdapterthat never callscreateOpenAIStreamAdapter. Measuring it there would mean standing up a backend SSE endpoint, which was not proportionate, so the node measurement is the evidence and this states its limits.Tests
A differential fuzz test drives random arrival streams, with tags split at every offset, tool boundaries, unclosed tags, empty arrivals and truncations, and requires the incremental parse to deep-equal a full reparse after every single arrival. The placeholder watch gains a soundness sweep: for every state of every stream, if the strip would cut, the watch admitted it.
Two source pins from #9012 are repointed rather than dropped. The one that asserted the exact spelling of the tracker call now asserts what that spelling protected, that the reply grows through one call, which is what keeps the delta-fed pieces in step. A new pin enumerates the forms in which the loop may mention the buffer at all, because character counting cannot see this defect: a
charCodeAtand a full scan read the same characters and cost the same, so only a rule about touching the buffer catches a regression.Every new test was run against a deliberately broken tree. One does not earn its place and is called out rather than quietly kept:
a think block split by a tool boundary parses as the adapter parses itpasses under all 14 breaks tried. It is a characterisation pin on existing behaviour and is documented as such.Full frontend suite 2973 pass, 0 fail. Typecheck clean.
Not done here
assistant-stream, already a dependency, exposes an append-basedappendText(textDelta), and migrating the adapter ontoAssistantStreamwould be the upstream-aligned form of this fix. That is a rewrite of a 6,000 line generator that would still needcumulativeTextfor the finalizers, the token estimate and continuation repair, so it is not attempted here.countReasoningGroupsandlastReasoningGroupTextLengthare deliberately left alone. They take the parts array rather than the text, and measure 0.87 to 1.76 ms over the same 55,000 character stream, about 5% of the flattening floor and about 1% of the parse this removes. They are O(1) per arrival and not worth touching.