Studio: give a CRLF reply the same incremental rendering as an LF one by danielhanchen · Pull Request #9088 · unslothai/unsloth · GitHub
Skip to content

Studio: give a CRLF reply the same incremental rendering as an LF one - #9088

Merged
danielhanchen merged 2 commits into
mainfrom
studio-crlf-incremental-cache
Aug 17, 2026
Merged

Studio: give a CRLF reply the same incremental rendering as an LF one#9088
danielhanchen merged 2 commits into
mainfrom
studio-crlf-incremental-cache

Conversation

@danielhanchen

@danielhanchen danielhanchen commented Aug 17, 2026

Copy link
Copy Markdown
Member

Problem

IncrementalMarkdownCache measures every boundary as an offset into the text it is
handed. It compares those offsets against the blocks parseMarkdownIntoBlocks
returns, and Streamdown hands those back with their line endings already
normalised. On a CRLF reply the two disagree one block in:

parseMarkdownIntoBlocks(remend("para one\r\n\r\npara two\r\n\r\n"))
// ["para one", "\n\n", "para two", "\n\n"]      the source holds "para one\r"

findCommitBoundary matches block 0 at offset 0, advances to 8, then asks whether
the tail continues "\n\n" where it actually holds "\r\n\r\n". The check fails,
repairBroke is set, and update takes the sticky full-document path. From
that point the whole reply is repaired and lexed on every frame, for the rest of
the reply.

A user whose provider or platform emits CRLF gets no incremental rendering at all.

It is not only a cost bug. Streamed at every prefix over a 454 reply corpus, a
CRLF reply's block list already differs from the same reply in LF in 11 of 454
replies on main, and the block count differs in one. After this change it does not
differ in any. So main renders some CRLF replies into a different block list than
the identical LF reply, which is a correctness difference, not a performance trade.
The rest of the difference is time, and nothing in the output says so, which is why
this has survived.

The change

Normalise the line endings at the entry to the cache.

Per the CommonMark spec, "a line ending is a
line feed (U+000A), a carriage return (U+000D) not followed by a line feed, or a
carriage return and a following line feed", and reference parsers normalise to LF
before parsing, so this cannot change what is rendered. A lone carriage return
is included deliberately: a CRLF pair can be split across two frames, and treating
only the pair would leave that CR in place for one frame and delete it the next,
which is one more retro-edit for the cache to absorb.

The scan is a native includes, and the replace runs only for a reply that actually
carries a carriage return.

Measurement

Paired A/B in a single process, both versions of the cache imported side by side and
run alternately so host load lands on both arms, 24 character frames, median of 5
paired ratios.

CRLF reply on main with this change the same reply in LF, for scale
8,520 chars, 355 frames 333 ms 68 ms (4.86x, 4.58 to 4.90) 59 ms
34,080 chars, 1,420 frames 20,177 ms 701 ms (28.65x, 23.07 to 29.11) 521 ms

The ratio grows with the reply because the discarded work is the whole reply each
time. After the change a CRLF reply sits within 1.35x of the same reply in LF; the
gap left is the per-frame preprocessing, not the cache.

Retention over a 454 reply corpus streamed at every prefix:

retained characters
LF on main 7,591
CRLF on main 281
CRLF with this change 7,591

Exact parity with the LF corpus.

Rendered output is unchanged

454 replies streamed at every prefix in both line endings, comparing the mdast text
and math nodes of the whole document, of every block on its own, and the block count:

comparison documentText documentMath blockText blockMath blockCount
LF: this change vs main 0 0 0 0 0
CRLF: this change vs the same reply in LF 1 85 0 0 0
CRLF vs LF on main today 1 85 1 11 1

Row 1 is the guarantee for the common case: an LF reply is not touched.

Row 2 is the goal, reached: a CRLF reply now renders as the identical block list to
the same reply in LF. The documentText and documentMath columns in that row are
not this cache: that fingerprint is a direct whole-document parse of the CRLF
text, bypassing the cache entirely, and row 3 shows the same 1 and 85 on main.
That is a pre-existing property of parsing CRLF markdown with micromark, unchanged
here in either direction.

Row 3 is the correctness point above, in the table: on main a CRLF reply's block
list diverges from the LF one in 11 replies and the block count in 1. Row 2 shows
both at 0.

The oracle, the cache's block list against a whole-document split at every prefix of
353 CRLF replies: 0 mismatches, comparing against a parse of the normalised text,
which is what a CommonMark parser sees.

Tests

studio/frontend/tests/streaming-crlf-retention.test.ts, three tests. Being explicit
about which are detectors, since a test that passes on the broken tree proves nothing
about the bug:

  • a CRLF reply retains as much as the same reply in LF - fails on main
    with CRLF retained 0 characters against LF's 10518. This is the defect. It also
    pins the block list, so retention that returned different blocks would not pass.
  • a CRLF reply matches a whole-document split at every prefix - passes on main
    as well. It is the output guard, including a reply ending in a lone carriage
    return, not a defect detector.
  • an LF reply is untouched by the line-ending handling - passes on main as
    well, by design: it is the regression guard for the common path.

npm test (3,590 pass), npm run typecheck, npm run build and npx eslint on
both changed files are clean.

Scope

Found while verifying #9017, and deliberately not folded into it: it is a separate,
pre-existing defect that predates that PR, and #9017's own Codex round raised a CRLF
issue in the blank-line scan whose stated CPU consequence turned out to be this,
rather than anything #9017 introduced.

IncrementalMarkdownCache measures every boundary as an offset into the text it
is handed, but compares against blocks Streamdown returns with their line
endings already normalised. On a CRLF reply the two disagree one block in:
findCommitBoundary sees "para" where the source holds "para\r", the exact
prefix check fails at the first separator, nothing is ever committed, and the
sticky full-document path repairs and lexes the whole reply on every frame.

Normalise the line endings on the way in. CommonMark counts a line feed, a lone
carriage return, and a carriage return followed by a line feed as the same line
ending and reference parsers normalise before parsing, so this cannot change
what is rendered; a lone carriage return is included because a CRLF pair can be
split across two frames.

Measured, paired in one process, 24 character frames: a 34,080 character CRLF
reply goes from 20,177 ms to 701 ms, against 521 ms for the same reply in LF.
Retention over a 454 reply corpus goes from 281 characters to 7,591, which is
exactly what the LF corpus retains. A reply with no carriage return in it takes
the same path it took before, byte for byte.
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

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

Reviewed commit: 0f2af576c1

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

The comment named indexOf; the code calls includes. Replace the primitive with
the measurement it was standing in for: 0.4 us per frame on an 88,000 character
reply.
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@danielhanchen

Copy link
Copy Markdown
Member Author

Independent re-review: no change needed to the fix

Re-reviewed against the strict bar. No genuine bug found; one comment corrected in
df84ffa (it named indexOf where the code calls includes, replaced with the
measurement it stood in for).

What was checked beyond the description:

  • Code literals are unaffected, and were already unaffected. Fenced blocks,
    indented code and code spans carrying CRLF and lone CRs were streamed at every
    prefix on both trees: byte-identical block lists, and no \r survives into a
    block on main either, because Marked's preprocessor strips it inside
    parseMarkdownIntoBlocks before this code ever sees it. The copy paths were read
    rather than assumed: message copy uses getCopyText() on the raw message store
    and never passes through this cache, and fenced-block copy is fed the post-Marked
    block string.
  • Invariants hold with rewinds actually firing. 4,368 prefix checks over CRLF,
    LF and lone-CR documents asserting source === committedBlocks.join("") + tail,
    no \r stored anywhere, committedLength equal to the joined block length, every
    commitPoints[i].length a real offset into source, and the block-list oracle,
    with rewoundCharacters at 5,135 so the rewind path was exercised throughout.
    Zero failures. CRLF retention comes out block for block identical to LF.
  • The normaliser is prefix-monotone. 8,640 prefix pairs over a CR-heavy
    alphabet, 0 violations. This is why the lone \r case has to be handled: without
    it a frame ending mid-\r\n would normalise differently from the next frame and
    hand the cache one more rewrite to absorb.
  • The guard costs 0.4 us per frame on an 88,000 character LF reply. On the CRLF
    path the per-frame replace is 46 us, against the 84 ms per frame it removes, and
    heap use across a full 3,801 frame sweep was flat.
  • No merge-order hazard with Studio: sweep the streaming markdown corpus at every prefix #9085, verified in both directions.

One thing this surfaced that is not in scope here

Cross-platform CI on a staging repo runs the frontend suite on Windows, which the
org workflows do not. It is red on main, not because of this PR: 13 failures, all
clipboard, notification-permission and marker-key tests (web build writes through navigator.clipboard before yielding, the marker key belongs to nothing else in the app, a browser tab calls no command, and 10 more). The identical 13 fail on this
branch and on #9085's branch, which are unrelated changes on two different staging
repos, so they come from main. Worth its own issue.

@chatgpt-codex-connector

Copy link
Copy Markdown

@danielhanchen
danielhanchen merged commit 8db7229 into main Aug 17, 2026
35 of 37 checks passed
@danielhanchen
danielhanchen deleted the studio-crlf-incremental-cache branch August 17, 2026 12:05
danielhanchen added a commit to Datta0/unsloth-staging-3 that referenced this pull request Aug 17, 2026
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