Studio: incrementally tokenize streaming code fences - #8935
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99a34e7f71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // tildes, then spaces. It also starts a line, so the body it leaves behind ends | ||
| // at a newline. The run can be short, because the cached code is the last text | ||
| // tokenized and the run may still have been arriving then. | ||
| const CLOSING_FENCE = /^ {0,3}(?:`+|~+)[ \t]*$/; |
There was a problem hiding this comment.
Match closing suffixes to the opening fence marker
When a large tilde-opened fence ends with a backtick-only content line (or vice versa) and a later same-language sibling contains only the preceding prefix, this pattern accepts the wrong marker family as a closing delimiter. findFence then shares the cache entry, and the shorter sibling can clear the longer sibling's pending refresh, leaving the longer block with a plain approximate tail. Markdown requires the closing fence to use the same marker as the opener, so preserve that marker for this check or keep these siblings separate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The plugin never sees the opening marker. HighlightOptions is {code, language, themes}, and the info string is identical for a tilde fence and a backtick fence, so there is nothing to match the suffix against. Keeping the siblings separate is not available either: a body of P plus a bare marker line shortening to P is byte-identical whether one fence just closed or two fences share a prefix, so no content-only rule tells them apart.
I did reproduce the mechanism, and it needs the same repeated render of unchanged code as the identical-fence item. One document-order pass over both fences returns exact tokens for each on this branch, and that pass is what the consumer performs, since its effect is keyed on the code. The identical-fence fix removed the cancellation for a caller whose code already equals the entry's; here the caller's code is shorter, so it does not reach. Leaving this rather than adding a check that cannot be correct.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The incremental tokenization here is correct, including for the cases the existing tests do not reach. These add coverage for the three gaps. Embedded grammars. Every existing fixture is python, json or typescript, where one grammar spans the whole fence, so resuming from a saved GrammarState is never asked to restore a stack more than one level deep. Five fixtures that are: HTML with embedded script and style, TSX with JSX children, markdown with a multi-line HTML comment, shell heredocs with runtime terminators, and nested template literals. The engine. The desktop app's CSP is default-src 'self' with no wasm-unsafe-eval, and the packaged frontend is this same bundle, so switching to shiki's Oniguruma engine would pass every test and every browser and fail only in the shipped desktop app. Nothing in CI builds the desktop bundle and drives its webview, so this pins the JS engine. The cache budget. MAX_CACHED_CHARACTERS is 512,000 and the largest existing fixture is about 36 KB, so the character-budget branch, its never-evict-the-only-fence carve-out and the running character count were all unreachable. Each test was checked against a deliberately broken tree rather than assumed to discriminate, and two did not until they were rebuilt: - The evict-then-resume test spent 22 of 23 comparisons on the throttled approximation, which renders the uncommitted tail plain and never reads the grammar state, so it could not fail however the resume broke. It now waits out REFRESH_MS between updates. - The same test used a python triple-quoted string, whose tokens are byte-identical with the resumed state dropped. It now uses HTML with an embedded script, where the grammar is several levels deep at the resume point, and fails at character 2907 of 4414 when the state is dropped. Tests only, no source changes. 32 passed, 0 failed on this head.
|
Reviewed this closely and it holds up. The mechanism is sound: the object being retained across updates is shiki's public I have pushed tests to the branch rather than only describing them, What was not coveredEmbedded grammars. Every existing fixture is python, json or typescript, where a single grammar spans the whole fence, so resuming from a saved state is never asked to restore a stack more than one level deep. That is the case most likely to diverge, and a divergence there produces plausible tokens with wrong scopes rather than an obvious break. Added five: HTML with embedded script and style, TSX with JSX children and an expression container, markdown with a multi-line HTML comment, shell heredocs with runtime terminators, and nested template literals. All match whole-document The regex engine. This is the one desktop-specific hazard in the file and it is worth stating explicitly: The cache budget. Two things worth knowing about testing this fileEach test was checked against a deliberately broken tree rather than assumed to discriminate, and two did not until rebuilt. Both traps are easy to fall into again, so they are written into the test comments with the measured numbers.
The existing fixtures all sit under Not changed, raised for your callThe React tree is still rebuilt every frame. Tokenization is incremental now, but Fence identity remains a heuristic. The plugin never receives the opening marker, so it cannot apply CommonMark's rule that a closer matches the opener's family and is at least as long. The unstyled tail is visible. During fast streaming of a large fence the tail renders plain for up to 250 ms. That is the intended tradeoff of the throttle, not a bug, but it is the behaviour users will notice on exactly the long code cells this targets, and it is currently undocumented. |
|
Picking this back up now that main has moved a long way underneath it. Three things: a current answer on the red check, the conflict, and two effects of this change that I do not think either of us had noticed. The failing check was stale, and the current answer is green
Full frontend suite on the merged tree: 3790 tests, 3788 pass, 2 fail. Both failures are unrelated to this change and neither test imports
Worth noting for anyone else checking this: The conflict, and one trap in itResolved and pushed. The conflict itself was small, both sides had edited the comment above I also fixed 5 This change fixes a real correctness bug, which the description undersellsThe plugin this replaces delegated to It reproduces on current main in a few lines. Two blocks of equal length sharing an opening and a closing but differing in the middle: This branch is immune, because
Two similar config or import blocks in one reply is a realistic shape for this, so I would call it reachable rather than theoretical. I have pushed a regression test for it ( It also closes an unbounded cacheThe same upstream cache is module scope and never evicts. Every intermediate prefix seen while a fence streams becomes a permanent entry holding a full tokenization of that prefix. Measured with
Linear in fence count on main (12.87 x 5 = 64.4, measured 65.4), bounded here by So the honest framing of this PR is broader than the title. The CPU reduction is the headline and I have measured it separately, but the change also removes a wrong-content rendering bug and an unbounded memory leak that exist in the tree today. Does it break anythingThe old pathways still work, checked rather than assumed. The behaviour this replaces is preserved on every axis I could find a way to test:
Still open from my earlier reviewThe fence-identity question is the one structural item I have not closed. This branch cannot see the opening fence marker, so Nice piece of work. The mechanism is sound and it turns out to be doing more good than it claims. |
|
Browser matrix, since this is a rendering path and the node suite only proves it in node. I bundled the plugin from this branch with esbuild and drove it in Chromium, Firefox and Playwright WebKit, streaming an HTML fence with embedded Six cut points, all strictly increasing and all inside the source, with a 300 ms wait before each so every one of them outlasts
The second row is the part that makes the first row mean anything. I built a second bundle from a tree with one line changed, The one cut that still matches on the broken tree is the first, at 2,100 characters, which is expected: Two honesty notes on this. WebKit here is Playwright's WebKit, which I am treating as a proxy for Safari and for the WebKitGTK that Tauri embeds on Linux. It is not real Safari and I am not claiming it is. And the engine question I raised earlier is now settled in the reassuring direction: |
|
Closing out the fence-identity question I left open, since it was the one structural item that could have blocked this. It does not block. But The heuristic cannot produce wrong tokensCommonMark 0.31.2 section 4.5 requires a closer to use the same marker family as the opener, with a run at least as long, at most three spaces of indentation and nothing after but spaces or tabs. It cannot corrupt tokens, though, and the reason is structural rather than lucky. The part I did not expect
Streamdown's lr = e => { let t = e.length; for (; t > 0 && e[t-1] === "\n";) t--; return e.slice(0, t) }and it is applied as That has a visible consequence, because the renderer's callback has no cancellation: let r = o.highlight({code: s, language: e, themes: l}, c => { i(c); });
The block permanently renders a stray
So this is pre-existing on main, not a regression, and not a reason to hold this PR. main has no equivalent guard at all; this branch added one that happens not to reach. Two things that followFirst, this is further support for the 👎 already on the marker-family item. Not only is the information unavailable at this layer, as you said, the predicate that would use it does not currently run. Matching the marker family would not change any observable behaviour today. Second, the fix is close but it is a deliberate trade rather than free, which is why I am flagging it rather than pushing it. The shape to match is The cheap thing worth doing here, if you want it, is a one-line comment above For completeness, on the question this PR does own: a 35 KB Python fence streamed over 300 frames measured 845/989/848 ms of process CPU on main against 269/221/259 ms here, so roughly 3.4x to 4.5x, consistent with the numbers I posted earlier at a different fixture size. Where the opening marker actually livesFor the record, since it will come up again: Streamdown does know it. |
|
Correcting one number in my previous comment, and adding the thing that makes it useful. I quoted "845/989/848 ms on main against 269/221/259 ms here, so roughly 3.4x to 4.5x" for a 35 KB Python fence. That figure came from a measurement I had not re-run myself before posting it, which is not the standard I have held the rest of this review to. So I measured it, three interleaved repetitions per cell, At that fixture size I reproduce the
So 2.7x to 2.9x, not 3.4x to 4.5x. The The more useful finding is that the ratio is strongly a function of fence size, which a single multiplier hides. Same harness, same pacing, only the fence doubled:
Doubling the fence roughly quintuples This is the same caution I raised earlier about the character-reduction figure being cadence-dependent, and it applies to the CPU figures too: quoting one multiplier invites someone to measure a different fence, get 2.7x, and think the work regressed. A range with the fixture attached ages better. The numbers in the PR description are fine on that front since they name their fixtures. Nothing here changes the verdict. It makes the case stronger at the sizes that motivated the PR. |
|
Rendering evidence, since this touches a rendering path and the premise is that the output does not change. Two separate Studio installs, one per tree, each with its own home and port, driven by the same scripted SSE bytes with no model loaded so both sides receive identical reply text. BEFORE is Two fences per run, each streamed 100 characters every 45 ms with three deliberate pauses mid-fence so the same prefix could be photographed on both builds fully refreshed:
Every checkpoint sits above The painted fence markup is byte-identical at every sample. Hashes over the live DOM of the fence, so this is token boundaries and colours rather than a visual impression:
Inside The control, which is the part that makes a parity pair mean anything. A pair of identical screenshots is also what you get from photographing one build twice, so the scene's
So the two halves are demonstrably two different builds producing the same pixels at a third of the main-thread cost. One divergence, named rather than smoothed over: in the mid-stream pair the images differ in a single 16x16 pixel box, the sidebar "generating" spinner caught at a different point of its CSS rotation. Wall clock, not content. Four of the six screenshot pairs are byte-identical PNGs. On the trap that a settled screenshot could mask a broken resume by re-tokenizing everything at the end: |
|
@codex review |
|
Follow-up review at head 1. The streaming content-loss report does not belong to this PRA streaming failure has been reported where text streams in, generation finishes, and then a large part of the rendered reply disappears. The build it was seen on had this PR and another in-flight streaming change applied together, so it could not tell the two apart. I ran the three arms separately. Method: same frozen
This PR alone loses nothing. It renders 12,273 of 12,273 characters and matches frozen main exactly, in 11 out of 11 trials. Arm C additionally never renders more than about a third of the reply in the first place, which is the larger of its two problems and one that a "lost after completion" number understates. I am not drawing a conclusion here about the other change on its own, since arm C is not a clean superset of arm B; the point I can support is the one about this PR. Two honest caveats. Chromium here is a proxy, not the packaged desktop webview. And the 2. The gaps I raised earlier are closed, and the new tests are proven non-vacuousA test that passes is not evidence until it can fail. I cut six worktrees from head, broke one thing in each, and re-ran the whole suite. Baseline at head is 37 pass, 0 fail, 0 skipped.
Four of those fail exactly one test while 36 others pass, which is the useful property: each guard is specific and nothing else in the suite can detect that regression. In particular the first one confirms what the performance test claims about itself, that re-tokenizing whole would pass the entire rest of the suite, so that test is the only thing standing between this PR and a silent loss of the property it exists for. On the fence-identity item: I agree with your reasoning and I am not asking for a change. The plugin genuinely cannot see the opening marker, and a body plus a bare marker line is ambiguous by construction. That is a limitation of the interface, not a defect in this PR. The engine assertion is worth one note. It pins source text rather than behaviour, which is unusual, but 3. Does merging break anythingThe PR touches one source file plus its tests. No storage, schema, migration or thread-persistence path is touched, so an existing Gates at head: 37/37 tests pass with 0 skipped, One small thing: the body still says "15 focused highlighting tests" and head now has 37. 4. Scope note on the reasoning paneSeparately measured, so it does not get misattributed to this PR later: incremental tokenization does not fix the slow streaming reasoning pane. Mounted span counts come out essentially identical with and without this PR (peaks within a handful of spans of each other). The pane renders through the same path, so this PR is structurally on it, but this PR changes the cost of producing tokens, not the number of spans mounted, and the pane's problem is the mount count. That is a separate piece of work and it takes nothing away from the CPU result here. Nice work on this one. The incremental design holds up under everything I could throw at it. |
|
@codex review |
A measurement that bears directly on what this PR has left to winI have been measuring the parse-side cost of a streaming reasoning reply for a separate change Shiki costs ~0.00 ms per streamed chunk, and the reason mattersPer chunk, on a reasoning-pane-shaped fixture at 90,000 characters (3,750 chunks of 24 characters, Shiki reads 0.00 on both builds. That is because every fence is tokenised once and then served I want to be careful about the scope of that. It says nothing about the FIRST tokenisation of a The number was reached by ablation, not by a wrapper, which is why I trust itA wrapper around So Shiki runs about 7% inside React's render phase and 93% outside it, in the trailing The 0.00 above is therefore not from that wrapper. It is from ABLATION: replacing the real What I would suggest measuring before landingSince the caching already removes the repeated work, the case for this PR probably rests on the Method and caveats: Chromium only; one run per cell; production numbers taken against a bundle |
The markdown fixture already here holds an HTML comment, which pushes one shallow level and leaves the nested fence's body doing no work. This one opens a python fence and a bash fence inside a markdown body and closes both, so the grammar switches in and back out twice. That matters because a `#` line inside a fence is body text and the same line outside one is a heading. A resume that stays a level too deep, or returns a level too shallow, colours them the other way round and drags the prose after each fence with it. Checked against a tree with `grammarState: fence.state` replaced by `undefined` rather than assumed to discriminate: it fails there at character 100, the `# Collect the rows` line inside the python fence, which the broken tree renders as an H1. The shared helper now waits out REFRESH_MS once a fixture is past MIN_INCREMENTAL_CHARS. Without it this test fails on correct code at the first comparison past the threshold, because the throttled approximation renders the uncommitted tail plain and never reads the grammar state. All 119 comparisons here take the resume path, 15 of them past the threshold. The five existing fixtures are all below it and pay nothing. Tests only, no source changes. 38 passed, 0 failed.
|
Came back to this with a specific question, and the answer turned out to be worth writing up properly because it nearly went the other way. The questionA separate piece of profiling on the streaming pane measured Shiki at approximately 0.00 ms per chunk, and attributed it to every fence being tokenized once and cached. Taken at face value that would undercut this PR: if fences are already tokenized once in the shipped path, making tokenization incremental would win much less than the numbers here suggest. I did not want to raise that with you as an inference, so I measured it. It does not undercut this PR. Two of the three parts of that claim are right, and the part that would have mattered is not. What the fixture behind that measurement actually isThe ablation ran on plain That is a fact about the fixture, not a criticism of the instrument. The ablation is sound for what it does measure, and I say below what that is. Measured, on both trees, same probeBoth trees import 1. The win does not start at the threshold, and it is not flatSingle TypeScript fence, streamed 24 characters every 73 ms, which is a real token rate. Medians of three.
I expected the two trees to be indistinguishable below 2,000 and predicted that in writing before running it. That was wrong, and wrong in your favour. There is no discontinuity at the threshold at all; the curve is smooth from 477 characters upward. The reason is worth stating in your description, because I had to read the source twice to see it: 2. On the profiled fixture itself, this PR is 3.9x cheaperSame 90,000-character fixture as the ablation, real 73 ms gap, three interleaved repetitions per arm.
My main-arm count of 1,649 calls over 21 fences lines up with the other instrument's 1,672 over 22 on the same fixture shape, the difference being one truncated fence at the slice boundary. Two independently built probes within 1.4% is the best evidence either of us has that both are on the real path. 3. Why 3.83x and not 41x, which is the honest limit of this designAttributing every call to a fence body:
The single-line fences cost both trees 358,851 characters, identical to the character, and they are 85% of everything this PR still spends. That follows directly from committing completed lines: a fence containing no newline commits nothing, so its live line is re-tokenized every frame on both trees. Your description already says one long line still relies on the existing throttle, and this is that, quantified. It means the aggregate ratio on any real stream is a property of the content mix, so an all-multi-line stream reads about 41x and an all-minified-JSON one reads about 1x. 4. Closed fences are not re-tokenized on either treeThis is the part of the caching claim that is true, and I want to be precise about who it belongs to. Driving That caching is So what does the 0.00 ms reading meanIt means Shiki is not the streaming pane's bottleneck, and that is correct. On that fixture None of that reaches your headline claims, which are a single large fence in the answer, measured on production builds. It also does not reach mine: my earlier browser A/B here was two Studio installs each serving One wrinkle that is yours to judgeWhen a chunk boundary splits the closing backtick run, the body transiently grows to include a partial delimiter and then shrinks, and this branch pays one extra near-full pass. Confirmed causally rather than inferred: at 1,800-character fences exactly one fence sheds a partial closing run and exactly that fence pays 1,864 characters, while at 2,500 none sheds and none pays over 142. Your worst case at close equals The metric I would addFor a single long growing fence the number that shows what this design buys is time to first highlighted token, not steady-state cost per chunk. On HousekeepingI pushed one more test, The red checks are not yours. All six were checked. Where I landI went looking for a reason this PR wins less than it claims and did not find one. The mechanism is sound, the parity evidence holds across three browser engines and on production Studio builds, the win is real at every fence size I could measure, and it extends below the threshold in a way the description undersells. The single-line case is a genuine limit and you had already named it. Two small things for the body when you next touch it: it still says "15 focused highlighting tests" and head now has 38, and it is worth saying that the improvement covers short fences too. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |

Main already renders large streaming fences incrementally. Above 2,000 characters, #7537 reuses highlighted lines, displays the new tail as plain text, and refreshes every 250 ms. However, each refresh still asks Shiki to tokenize the entire block. If that takes longer than 250 ms, the throttle stops helping. Shorter fences are tokenized in full on every frame.
This PR makes the tokenization incremental too. It caches completed-line tokens and the grammar state after them, so each refresh processes only new text. The existing plain-tail cadence remains unchanged, so user-visible rendering does not change.
What changed
Measurements
Production builds were run alternately on the same machine, with three paired runs per row. The table reports medians from identical streamed updates.
Absolute totals vary between sessions, but the paired TypeScript reduction repeated at 40% to 52% at 1x and 86% to 87% at 4x.
Tokenizer instrumentation confirms the source of the improvement:
A replay through the real chat UI showed the same pattern. TypeScript main-thread CPU fell from 6,420 to 4,498 ms at 1x and from 81,348 to 19,646 ms at 4x. HTML moved from 4,532 to 4,309 ms. The gain depends on grammar cost and completed lines; one long line still relies on the existing throttle.
Rendering parity
codeToTokensoutput.Tradeoff
The cache is bounded, unlike the
@streamdown/codecache. Mounted blocks are unaffected; only evicted blocks that later remount are tokenized again. Re-mounting 250 fences took 0.7 ms on both builds, while 400 fences beyond the budget took 974 ms here versus 1.9 ms on main. This is the cost of avoiding unbounded retention of streamed prefixes.Out of scope
preprocessLaTeXcan still rewrite LaTeX-like text inside an open streaming fence until its closing delimiter arrives. That behavior already exists on main and is separate from highlighting.Checks
npm test: 2,717 passed, including 15 focused highlighting testsnpm run typechecknpx eslinton the changed filesnpm run buildfd6ab9a