Studio: stop the streaming prefix checks scanning the whole reply - #9038
Conversation
cef9b2d to
f38559f
Compare
Four checks on the streaming path ask whether the new text still begins
with what was already handled. All four used startsWith, which scans.
Slicing to the prefix length and comparing lets V8 reject on length and
then compare natively.
Measured over a 60,000 character stream, 1,052 comparisons, median of 5
interleaved runs:
strings sharing a parent 79.8 ms -> 1.1 ms
strings not sharing one 74.3 ms -> 1.6 ms
Both cases are given because a shared-parent microbenchmark can
exaggerate this; here it does not, the effect survives flattening.
Call sites: three in streaming-render-schedule.ts, one of them added by
the retained-prefix work, and the coalescer in markdown-text.tsx whose
own comment already put its scan at 59 ms across a 175,000 character
stream.
The substitution is exact. slice clamps, so a prefix longer than the
string yields a short slice that cannot equal it.
markdown-streaming-scheduling.test.ts asserted the literal spelling
"text.startsWith(displayed.text)", so it failed on a change that keeps
the behaviour identical. It now asserts both halves of the gate and
accepts either spelling: the length rejection has to be there, since the
compare alone would run on every arrival, and the prefix compare has to
be there, since the length check alone would pass a comparison against
the wrong string.
Testing: an equivalence fuzz, 20,000 randomised cases plus a fixed
corpus and every cut of astral text, asserting agreement with startsWith
throughout. It requires at least 4,000 of the cases to be real prefixes,
so it cannot pass by rejecting everything on length.
Mutation results, stated per mutation:
off-by-one on the slice caught, 3 of 3 fail
comparison in the wrong order caught, 3 of 3 fail
length guard removed NOT caught, and cannot be: slice clamps,
so that mutation is semantically
identical and only loses the fast path
npm test 2,946 passed, 0 failed. typecheck clean. biome adds no errors.
f3e4865 to
1084a92
Compare
|
Branch state note, to avoid a duplicate rebase. This branch is already rebased onto current The conflict with #9088 was real and is resolved. The branch also now carries two follow-ups from verification:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
The scheduling test accepted either spelling of the coalescer gate, and the previous code satisfies the startsWith half, so it passed unchanged on a tree with this PR fully backed out. Pin the slice form and reject the scanning one; verified by reverting markdown-text.tsx to the old spelling, which now fails where it passed 5 of 5 before. Nothing covered the three call sites in the cache at all. The two spellings return the same answer, so no output test can separate them and reverting the call sites was free. Assert instead that no one-argument startsWith survives on that path; the one call the helper cannot express takes a start position and compares a fixed block, so it does not grow with the reply and is excluded by the same rule rather than by an exception. Verified by reverting two call sites, which now fails. Drop the duplicated measurement at the coalescer and point at the helper.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
Verification against current mainRe-verified end to end after main moved: 9085 and 9088 merged at 12:06Z. 9088 matters here more than anywhere else, because it edits this same file and rewrites Is it the same function
Is the rendered output the sameRecorded SSE from 33 cases across Chromium, WebKit and Firefox: all rendered text identical. Cases: ordinary reply, unterminated fence, 13,655 character reply, no fences, only fences, CJK, RTL, mixed RTL with CJK and code, reasoning, one token, empty, plus reload. Driven through the real What the screenshots showTwo isolated installs, merge base 34c9d98 against this head, one scene, the same recorded reply truncated mid-reply at SSE event 883 so the settled state ends on an unterminated python fence.
Both halves end on a fence the model never closed, which Studio has still rendered as a labelled, highlighted code block, with the interrupted notice, Continue and Retry. Read from the DOM, equal on both sides:
The two settled screenshots are byte-identical PNG files. Only the clocks moved: 8.5s against 8.47s. The pair was captured at 1084a92. The one source change since is comment-only in Cost, and a correction to what the comment used to claimThe published attempt at this substitution, bytes.zone, failed to beat
The effect tracks the prefix length, not whether the strings share a parent: cons and flat receivers differ by 250x against 254x, which is nothing. The doc comment used to attribute it to representation and now says this instead, including the warning not to reach for the helper on short prefixes. End to end through the real cache, 7 repetitions each, median:
The CRLF arm only exists because 9088 landed. Before it, a CRLF reply never committed anything and took the full document path every frame, so this prefix check was not on the critical path at all. Now it is, and this halves it. Tests, and a test that was measuring nothing
Fixed in 70e333c: the slice form is pinned and the scanning form is asserted absent, verified to fail on the reverted tree. The three call sites inside the cache had no coverage at all, and since the two spellings return the same answer no output test can reach them, so there is now a source assertion that no one-argument Mutation tested with 8 planted bugs: 8 caught, none survive. What was not tested
Cross-platformStaging CI on shimmyshimmer/unsloth-staging-4, since the org queue is carrying 34 pending checks per PR. Green on ubuntu-latest, macos-14, windows-latest, frontend ubuntu-latest and frontend macos-14. |
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 |
1 participant
Four checks on the streaming path ask whether the new text still begins with what was already handled. All four used
startsWith, which scans. Slicing to the prefix length and comparing lets V8 reject on length and then compare natively.Stacked on #9017, which restructures the same file and added one of these call sites.
Measured
60,000 character stream, 1,052 comparisons, median of 5 interleaved runs, Node:
startsWithBoth cases are given because a shared-parent microbenchmark can exaggerate this. Here it does not: the effect survives flattening, so it is not an artifact of rope structure.
Call sites: three in
streaming-render-schedule.ts, one of them added by #9017, and the coalescer inmarkdown-text.tsxwhose own comment already put its scan at 59 ms across a 175,000 character stream.The substitution is exact
sliceclamps, so a prefix longer than the string yields a short slice that cannot equal it. There is no input on which the two disagree, which is the whole point and also the thing worth testing.A contract test asserted the spelling, not the behaviour
markdown-streaming-scheduling.test.tsasserted the literal stringtext.startsWith(displayed.text), so it failed on a change that keeps the behaviour identical. It now asserts both halves of the gate and accepts either spelling:Testing
An equivalence fuzz: 20,000 randomised cases over an alphabet including newlines, backslashes, backticks,
$, an astral character and a combining mark, plus a fixed corpus and every cut point of a string whose characters straddle UTF-16 code-unit boundaries. It requires at least 4,000 of the cases to be real prefixes, so it cannot pass by rejecting everything on length.Mutation results, per mutation rather than in aggregate:
The third is a genuine zero, not a coverage gap. Because
sliceclamps, removing the length guard is semantically identical and only loses the fast path, so no behavioural test can detect it. Reporting it rather than dropping it, since a mutation table with only kills in it invites the reader to assume the rest were tried.npm test2,946 passed, 0 failed.npm run typecheckclean.biome checkadds no errors on any changed file.