Studio: stop rereading the whole reply on every streamed arrival - #9012
Conversation
Two scans in the chat stream adapter read the entire accumulated reply
once per SSE arrival, so both cost O(reply^2) over a generation and the
tail of a long answer streams slower than its head.
The first is the trailing `${...}` strip that external providers need.
The pattern is anchored at the end, but the engine still tries it at
every start offset in the buffer. It now runs over a bounded suffix,
trimmed further at the previous `}` because `[^}]*` cannot span one, and
is skipped outright unless the last non-whitespace character is `}`.
Behaviour is unchanged for every fragment up to the window; a longer one
is left whole rather than half removed, which is the safe direction since
the scan can then only strip less than the pattern would, never more.
The second is `hasUnclosedThinkTag(cumulativeText)`, two `lastIndexOf`
calls over the whole buffer. A small tracker keeps the last position of
each tag and looks only at what arrived since the previous call plus the
seven characters in front of it, which is the most a tag split across
arrivals can hide behind, so a `<think>` delivered one character at a
time is still found on the arrival that completes it. It also handles the
strip above taking a tag away with the suffix it removes, and is called
as its own statement rather than from inside the short-circuiting
condition, so it sees every state the buffer passes through.
Measured over a 55K reply of 13,752 arrivals, medians of 9 repetitions:
the strip falls from 1052 ms to 15.7 ms and the think-tag check from
180 ms to 15.9 ms. Both remainders are the cost of touching the growing
buffer at all, which the rest of the loop pays anyway: one `charCodeAt`
per arrival over the same stream is 15.1 ms.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc566bc2d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const tail = text.slice(from); | ||
| const previousBrace = tail.lastIndexOf(CLOSE_BRACE, end - 2 - from); | ||
| const scanFrom = previousBrace === -1 ? 0 : previousBrace + 1; | ||
| const match = TRAILING_TEMPLATE_PLACEHOLDER.exec(tail.slice(scanFrom)); |
There was a problem hiding this comment.
Leave oversized nested placeholders intact
When an oversized candidate contains another ${ within the final window, this bounded slice loses sight of the earlier opener and the regex strips only the nested suffix. For example, "answer ${" + "a".repeat(4196) + "${nested}" returns the unterminated outer fragment ending in the repeated as instead of leaving the oversized fragment whole as documented. External streams containing nested template syntax can therefore still have model-written text deleted and be left with malformed output; detect this ambiguous cutoff case and decline to strip it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked this against the example given, and it comes out the other way round.
"answer ${" + "a".repeat(4196) + "${nested}" (4,214 chars):
| kept | deleted | |
|---|---|---|
main (unbounded pattern) |
6 (answer) |
4,208 |
| this PR (bounded scan) | 4,205 | 9 (${nested}) |
So nested template syntax in an external stream loses 4,208 characters of model-written text on main today, and 9 with this change. The bounded scan strips strictly less here, not more, and the 9 characters it does remove are a complete, well-formed trailing ${...}, which is exactly what this function exists to remove. The result is still a prefix of the input.
The safety property the PR claims is that the bounded scan can never remove more than the unbounded pattern would. That is asserted directly across windows of 1, 2, 4, 8 and 16 characters, and it holds in this example. Declining to strip here would leave a complete trailing placeholder on screen for no gain.
There is a fair point buried in it though: the comment says an oversized fragment is "left whole", and in the nested case the outer fragment is not literally left whole, since its trailing placeholder is removed. That wording is imprecise and I am tightening it. The behaviour is intended.
The comment said an oversized fragment is left whole. That is the usual consequence rather than the rule, and nested placeholders show the difference: with the outer opener out of window, the inner one is what gets stripped. The guarantee that actually holds is about removal, so say that instead, and give the nested case as the worked example.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
…ceholders (#9091) * Pin what the trailing-placeholder window does with nested placeholders The module comment says the window's guarantee is about what is removed rather than about leaving an oversized fragment whole, and gives nested placeholders as the case where those two readings come apart. Nothing pinned it, so changing the behaviour to match the looser reading would have gone unnoticed. The test kills both directions: widening the window so the outer opener comes back into range, and declining to strip whenever an opener predates the window, which is the change a review of #9012 asked for. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: tighten the comments in the nested placeholder test --------- Co-authored-by: danielhanchen <unslothshared@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>



Problem
Two scans in
studio/frontend/src/features/chat/api/chat-adapter.tsread thewhole accumulated reply once per SSE arrival, so each costs O(reply^2) over a
generation. That is the shape users describe as "it gets slower the longer it
goes": the tail of a long answer streams noticeably slower than its head.
The trailing template-literal strip, on every arrival of an external
provider stream:
The pattern is anchored at the end, but a regex engine still attempts it at
every start offset in the buffer, so the cost is proportional to everything
written so far.
hasUnclosedThinkTag(cumulativeText), which islastIndexOf("<think>")and
lastIndexOf("</think>")over the whole buffer, evaluated while thereasoning-duration timer is deciding whether a group has closed.
The change
The strip moves to
src/features/chat/utils/trailing-template-placeholder.tsand runs the same pattern over a bounded suffix:
The last non-whitespace character has to be
}or the pattern cannot match.Almost every arrival stops here, for the cost of the trailing whitespace run,
which is walked no further than the window so a reply ending in a growing
field of whitespace cannot reintroduce the quadratic term.
[^}]*cannot span a}, so the opening${has to sit after the previous}. The suffix is trimmed there as well, which keeps brace-heavy replies,the ones that reach this far, far below the window.
The result is identical to the unbounded pattern for every fragment up to
TRAILING_PLACEHOLDER_WINDOW(4096) characters. A longer one is left wholerather than half removed. That is the only safe direction: the bounded scan
can strip less than the pattern would, never more, so it can never delete
text the model wrote. The property is asserted directly across windows of
1, 2, 4, 8 and 16 characters.
The bound is also worth stating the other way round, because it is not only a
risk to weigh.
[^}]*in the unbounded pattern will span the entire reply, soon
maina reply that opens a${and happens to end on a}loseseverything in between. A 5,087 character answer that begins "In JavaScript,
${opens a placeholder" and closes on${name}is cut to its first 16characters, deleting 5,071 characters the model wrote, on any external
provider. The window caps that: the two behave identically while the span is
under
TRAILING_PLACEHOLDER_WINDOW, and past it the bounded scan declines tostrip at all rather than eating the reply.
${and the final}So this is a bound on existing data loss, not a new approximation. It does not
remove the loss for a span shorter than the window; that is unchanged.
The think-tag check becomes
createThinkTagTracker()inparse-assistant-content.ts, next to thehasUnclosedThinkTagit replaces.It keeps the last position of each tag and looks only at what arrived since
the previous call, plus the seven characters in front of it, which is the most
a tag split across arrivals can hide behind. A
<think>delivered onecharacter at a time over seven arrivals is found on the arrival that completes
it. Two further cases the buffer actually produces:
buffer shorter than it was. The tracker re-finds only the tags that left,
which for an ordinary strip of plain text is no work at all.
the last term of a short-circuiting
&&chain. Behind the short-circuit itwould miss arrivals, and a strip on a missed arrival would leave it holding
positions for text that is gone.
Testing
All from
studio/frontend.npm test: 2957 passing, 0 failing.npm run typecheck: clean.npm run build: clean.npx eslinton the changed files: no new findings.chat-adapter.tshas 13pre-existing errors on
main(restricted imports, one unused import, threeuseless try/catch); the count and the lines are unchanged by this PR. The new
files are clean.
Cost, measured
The new tests are complexity tests, not timing tests. They count the characters
each scan looks at, through spies on
String.prototype.replace / lastIndexOf / indexOf / sliceandRegExp.prototype.exec / test, so they read the same on anidle machine and a loaded one. Twice the reply means twice the arrivals, so a
scan whose per-arrival cost does not depend on what came before doubles.
Growth for twice the reply: 3.99x before, 1.83x and 1.72x after.
Wall clock over the same 55K reply of 13,752 arrivals, medians of 9 repetitions
(this host is heavily loaded, so the ratios are the result and the milliseconds
are context):
The reply here is prose. The strip's before side is strongly shape dependent, so
the millisecond column should be read as one shape rather than a headline: over
the same 55K, prose measures 284 ms, brace-heavy text 352 ms, and a reply ending
in a growing whitespace field 10,440 ms. The after side stays between 14 ms and
140 ms across all of them. Where the strip fires on every arrival the buffer
never grows, the unbounded pattern is trivially cheap, and the bounded scan is
about 1.3 ms slower across 11,000 arrivals; that case is a small regression, not
an improvement.
${...}strip<think>checkBoth remainders are the cost of touching the growing buffer at all rather than
of the scan: a single
charCodeAtper arrival over the same stream measures15.1 ms, and the loop with no scan at all measures 0.1 ms. Repeated at 27.5K
(56x, 10.2x) and 110K (73x, 12.2x); the before side grows 4x per doubling and
the after side grows with the flattening floor.
Every new test shown to fail
Against
main's behaviour behind the same API (the bounded strip replaced bythe unbounded pattern, the tracker replaced by a full rescan, the adapter left
as it is), 6 of the 21 fail:
The other 15 are parity tests, which must pass on both trees by construction,
so they were checked against 14 deliberately broken trees instead. Every one of
the 21 tests fails under at least one:
}, not after itThe zero is reported rather than dropped: starting the scan at the previous
}instead of after it turns out to be equivalent, because
[^}]*cannot matchthat character anyway, so no test should have caught it.
That pass found two of the new tests to be vacuous, both from the same cause:
the seeded generator they used was
seed * 1103515245, whose product runs past2^53, so the low bits it was sampled on came out constant and half the alphabet
was never drawn. All 20,000 "random" strings were ones the pattern could not
match. Both generators now use mulberry32, and both tests assert that a real
share of their inputs reach the code path, so a scan that returns its argument
cannot satisfy them again.
Split tags are covered exhaustively rather than by example: every way of cutting
<think>and</think>into two, three and four non-empty pieces, plus onecharacter per arrival, plus empty arrivals between the characters of a tag, each
compared against
hasUnclosedThinkTagafter every chunk.Not addressed here
liveAssistantContent()runsparseAssistantContentover the whole buffer onevery arrival too. On the same 55K fixture it measures 37 ms, and it is not
quadratic for text with a
<think>block near the start, becauseindexOfstops at the first match; for a reply with no tags at all it would be. It is a
separate change.
The floor both scans now sit on is V8 flattening the accumulated string.
+=builds a rope, and the first read of the buffer after an append flattens it, so
any code that reads the reply once per arrival pays O(reply) per arrival no
matter how little of it it looks at. Removing that means not touching the
buffer per arrival at all, which is a larger restructuring of the loop.