Studio: stop rereading the whole reply on every streamed arrival by danielhanchen · Pull Request #9012 · unslothai/unsloth · GitHub
Skip to content

Studio: stop rereading the whole reply on every streamed arrival - #9012

Merged
danielhanchen merged 4 commits into
mainfrom
perf-chat-adapter-scans
Aug 17, 2026
Merged

Studio: stop rereading the whole reply on every streamed arrival#9012
danielhanchen merged 4 commits into
mainfrom
perf-chat-adapter-scans

Conversation

@danielhanchen

@danielhanchen danielhanchen commented Aug 16, 2026

Copy link
Copy Markdown
Member

Problem

Two scans in studio/frontend/src/features/chat/api/chat-adapter.ts read the
whole 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.

  1. The trailing template-literal strip, on every arrival of an external
    provider stream:

    cumulativeText = cumulativeText.replace(/\s*\$\{[^}]*\}\s*$/, "");

    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.

  2. hasUnclosedThinkTag(cumulativeText), which is lastIndexOf("<think>")
    and lastIndexOf("</think>") over the whole buffer, evaluated while the
    reasoning-duration timer is deciding whether a group has closed.

The change

The strip moves to src/features/chat/utils/trailing-template-placeholder.ts
and 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 whole
    rather 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, so
    on main a reply that opens a ${ and happens to end on a } loses
    everything in between. A 5,087 character answer that begins "In JavaScript,
    ${ opens a placeholder" and closes on ${name} is cut to its first 16
    characters, 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 to
    strip at all rather than eating the reply.

    span between ${ and the final } deleted before deleted after
    4,000 4,004 4,004
    4,090 4,094 4,094
    4,096 4,100 0
    20,000 20,004 0

    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() in
parse-assistant-content.ts, next to the hasUnclosedThinkTag it 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 one
character at a time over seven arrivals is found on the arrival that completes
it. Two further cases the buffer actually produces:

  • The strip above can take a tag away with the suffix it removes, leaving the
    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 call is now its own statement, immediately after the strip, rather than
    the last term of a short-circuiting && chain. Behind the short-circuit it
    would 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 eslint on the changed files: no new findings. chat-adapter.ts has 13
    pre-existing errors on main (restricted imports, one unused import, three
    useless 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 / slice and RegExp.prototype.exec / test, so they read the same on an
idle 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.

reply arrivals strip, chars scanned think, chars scanned
16K 4,002 64,080,020 -> 184,528 64,080,020 -> 132,030
32K 8,002 256,160,020 -> 427,058 256,160,020 -> 264,030
55K 13,752 756,525,020 -> 781,972 756,525,020 -> 453,780

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.

scan before after ratio
trailing ${...} strip 1052 ms 15.7 ms 67x
unclosed <think> check 180 ms 15.9 ms 11.3x

Both remainders are the cost of touching the growing buffer at all rather than
of the scan: a single charCodeAt per arrival over the same stream measures
15.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 by
the unbounded pattern, the tracker replaced by a full rescan, the adapter left
as it is), 6 of the 21 fail:

✖ the trailing placeholder strip is linear in the reply length
  strip cost grew 3.99x for twice the reply (64176120 -> 256352120 chars scanned)
✖ think tag tracking is linear in the reply length
  tracker cost grew 3.99x for twice the reply (64176120 -> 256352120 chars scanned)
✖ the adapter strips the trailing fragment through the bounded scan
✖ the adapter asks the tracker once per arrival, not inside a condition
✖ a placeholder past the window is left whole, never cut in half
✖ a whitespace run past the window cannot restart the whole-buffer scan

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:

broken tree tests it breaks
strip does nothing 5
strip falls back to the unbounded pattern 3
whitespace walk unbounded again 1
strip cuts at the window offset, dropping the brace offset 4
strip starts the scan at the previous }, not after it 0
tracker keeps no overlap between arrivals 7
tracker takes the first tag in the window, not the last 1
tracker ignores a removed suffix 2
tracker rereads the whole buffer 1
tracker records only the first tag it sees 4
tracker matches a prefix of the opening tag 6
tracker skips the first character of the buffer 8
adapter back to the inline pattern 2
adapter asks the tracker from inside the condition 1

The zero is reported rather than dropped: starting the scan at the previous }
instead of after it turns out to be equivalent, because [^}]* cannot match
that 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 past
2^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 one
character per arrival, plus empty arrivals between the characters of a tag, each
compared against hasUnclosedThinkTag after every chunk.

Not addressed here

liveAssistantContent() runs parseAssistantContent over the whole buffer on
every 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, because indexOf
stops 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.

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: f925919b4b

ℹ️ 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".

danielhanchen pushed a commit to danielhanchen/unsloth-staging-2 that referenced this pull request Aug 17, 2026
danielhanchen pushed a commit to danielhanchen/unsloth-staging-2 that referenced this pull request Aug 17, 2026
@danielhanchen

Copy link
Copy Markdown
Member Author

@danielhanchen
danielhanchen merged commit 564035f into main Aug 17, 2026
36 of 37 checks passed
@danielhanchen
danielhanchen deleted the perf-chat-adapter-scans branch August 17, 2026 10:41
danielhanchen added a commit that referenced this pull request Aug 18, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant