Keep the wide-cell invariant on every path that writes cells - #94
Conversation
Perf comparison3 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.
Each corpus is gated at assemblies measured
Regressions
|
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness regressions/incompletions in the changed areas (notably OSC payload handling, tab-stop completeness, and HasWideCells propagation) that can break Unicode OSCs and undermine the wide-cell invariant guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR is part of a correctness audit to enforce the “wide-cell invariant” across all buffer-writing paths (preventing split/orphaned halves of double-width glyphs), while also tightening parser conformance and input/resource bounds.
Changes:
- Enforces wide-cell integrity during erases and printing via
BufferLine.Fill, wide-cell repair helpers, and additional buffer state tracking. - Improves escape-sequence parser conformance (CAN/SUB abort handling, C1 ST terminating OSC, DEL handling, CSI param saturation) and adds OSC payload bounding.
- Introduces real tab stops (HTS/TBC) and adds/extends targeted regression tests across cursor/margins, parser, hostile inputs, and wide-cell behavior.
File summaries
| File | Description |
|---|---|
| src/XTerm.NET/Terminal.cs | Adds real tab-stop storage/reset and integrates it with HT handling and resize/reset behavior. |
| src/XTerm.NET/Parser/EscapeSequenceParser.cs | Parser conformance fixes (state transitions, OSC termination, saturation) and bounds on OSC growth. |
| src/XTerm.NET/InputHandler.cs | Wide-cell split repair on print/run paths, clustering/ZWJ fixes, cursor/margin behavior fixes, and protocol guards. |
| src/XTerm.NET/Graphics/PngDecoder.cs | Bounds zlib inflation during PNG decode to prevent decompression bombs. |
| src/XTerm.NET/Common/CommandExtensions.cs | Maps additional ECMA-48 cursor motion aliases (HPA/HPR/VPR). |
| src/XTerm.NET/Common/ClusterTable.cs | Caps interned cluster growth without ConcurrentDictionary.Count overhead on hot paths. |
| src/XTerm.NET/Buffer/TerminalBuffer.cs | Extends saved-cursor state (origin mode, pending wrap) and supports restoring pending-wrap. |
| src/XTerm.NET/Buffer/BufferLine.cs | Adds HasWideCells latch and widens erase spans to avoid splitting wide chars. |
| src/XTerm.NET.Tests/WideCellInvariantTests.cs | New tests covering wide-cell orphan prevention and cluster/ZWJ width rules. |
| src/XTerm.NET.Tests/Parser/ParserConformanceTests.cs | New parser conformance tests (CAN/SUB, C1 ST, DEL, malformed CSI, reset). |
| src/XTerm.NET.Tests/HostileInputBoundsTests.cs | New bounds tests for pathological inputs (OSC growth, PNG bomb, overflow params, etc.). |
| src/XTerm.NET.Tests/CursorAndMarginTests.cs | New behavior tests for cursor/margins/tab stop semantics vs xterm. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| else if (code >= 0x20 && code < 0x80) | ||
| { | ||
| OscPut(code); | ||
| } |
| // Done here rather than inside ResolveAutowrap because that helper answers "was the | ||
| // wrap resolved", and the OSC 66 sized-block path depends on its false to move a whole | ||
| // block back by its own width instead of one column. | ||
| _buffer.SetCursorRaw(WrapLimit(), _buffer.Y); |
| public void SetCell(int index, ref BufferCell cell) | ||
| { | ||
| if (index >= 0 && index < _length) | ||
| { | ||
| if (cell.Width == 2) | ||
| HasWideCells = true; | ||
|
|
||
| _cells[index] = cell; |
| { | ||
| // CHT - Cursor Forward Tabulation (CSI I) | ||
| var count = Math.Max(parameters.GetParam(0, 1), 1); | ||
| var tabWidth = _terminal.Options.TabStopWidth; | ||
|
|
||
| for (int i = 0; i < count; i++) | ||
| { | ||
| var nextTabStop = ((_buffer.X / tabWidth) + 1) * tabWidth; | ||
| _buffer.SetCursor(Math.Min(nextTabStop, _terminal.Cols - 1), _buffer.Y); | ||
| } | ||
| for (var i = 0; i < count; i++) | ||
| _buffer.SetCursor(_terminal.NextTabStop(_buffer.X), _buffer.Y); | ||
| } |
JohnCampionJr
left a comment
JohnCampionJr
left a comment
There was a problem hiding this comment.
Two paths still violate the wide-cell invariant and should be addressed before merge. The reported unicode performance regression also remains a deliberate merge decision.
| // A wide character needs TWO columns, so the wrap test has to know its width: written at | ||
| // the last column it was stored there with no room for its spacer, leaving a width-2 cell | ||
| // in one column and the cursor one past the pending-wrap position. | ||
| if (width == 2 && _buffer.X == wrapLimit && _terminal.Options.Wraparound) |
There was a problem hiding this comment.
This only forces an early wrap when DECAWM is enabled. With wrapping disabled, a width-2 character at the right margin proceeds to SetCell, while the spacer write is rejected by the margin check, recreating exactly the orphaned width-2 cell this PR is intended to eliminate. Please define the no-wrap behavior for a glyph that cannot fit (for example, reject or replace it without storing width 2) and add the same last-column test with DECAWM off.
| /// true is doing a check that finds nothing. Clearing it accurately would mean scanning the | ||
| /// row on every erase, which is the work it was added to avoid. | ||
| /// </summary> | ||
| public bool HasWideCells { get; private set; } |
There was a problem hiding this comment.
The latch is set only by SetCell, but wide cells also enter a line through CopyCellsFrom, Clone, and CopyFrom, all of which write _cells directly. Margin scrolling and reflow can therefore copy a wide cell into a line whose latch remains false; subsequent Fill/print repairs skip that line and can orphan it again. Please propagate/recompute HasWideCells in every bulk-copy path (and cover a cross-line copy in the tests).
edd3d18 to
375a68e
Compare
Perf comparison — this change, against its base3 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.
Each corpus is gated at assemblies measured
Worth a look — over the floor, under this run's gate, so not failed:
Re-run on a quieter machine, or with more Perf comparison — cumulative, everything since 2.0.0-rc0023 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.
Each corpus is gated at assemblies measured
Regressions
|
b0a86f6 to
a2be08c
Compare
|
Rebased onto main (post-#100) — this branch carried eleven commits that were already upstream as squashed merges, so only its own six replay. 1970 tests green. The two review findings, both realThe latch was set only by A wide character with DECAWM off. Also right: the early wrap is guarded by wraparound, so with it off the character reached the last column and was stored there while the margin refused its spacer — the same orphan, by the one path that skipped the guard. xterm.js parks the cursor and drops the character; so does this now. Both have regression tests that fail against the previous commit, along with the cross-line copy you asked for. Copilot's Perf: what actually happenedThe gate failed on
This run is green, and I do not think it should be read as fixed. Worth deciding rather than merging on it: raise |
On the
|
| base | head | delta | |
|---|---|---|---|
| run 3 | 33.74 | 35.32 ns/char | +1.58 ns (+4.7%) |
| run 4 | 32.05 | 33.80 ns/char | +1.75 ns (+5.5%) |
So the invariant costs about 1.7 nanoseconds per printed character. In terms anyone would notice:
- a full 80×24 screen of solid CJK — 960 two-column characters — costs 1.6 microseconds more to parse and store
- a megabyte of CJK text, roughly 333,000 characters, costs 0.55 milliseconds more
- throughput goes from about 31.2M to 29.6M characters per second
Nothing renders in that time, and nothing waits on it. The percentage is large because the denominator is small: unicode already runs at ~33 ns/char against ~3.5 for scroll-ascii, because shaping and width resolution genuinely cost ten times what a fixed-advance ASCII run costs. Five percent of the most expensive path in the library is still a rounding error against a single frame.
What it buys
An orphaned wide cell is not a cosmetic defect. A Width = 2 cell whose second column holds a real character makes the renderer draw a two-column glyph into one column, and every character to its right on that row shifts. It reads as corruption rather than as one wrong cell, and before this change it could be produced by ordinary editing: printing over either half, erasing a range that cuts through a pair, deleting a neighbour, a wide character at the last column, one at a DECSLRM right margin, or one arriving through a bulk copy during margin scrolling or reflow.
Why the corpus is the worst case, not the average one
unicode is built to be pathological, and its own description says so — "expect this to be the slowest stream by a wide margin." Each line shuffles seven pieces, five of them wide. Simulating the generator over 20,000 lines: 71.6% of lines have their first wide character at column 0, the median first-wide column is 0, and a filter that skipped work before the leftmost wide character could skip only ~3% of each line.
That matters because the guard which makes this free everywhere else — "this line has never held a wide character" — is true from the first character on every line of CJK. Every other corpus is flat precisely because that guard works there. Realistic mixed content — a log with occasional CJK, source with an emoji in a comment, a prompt with one icon — sits far closer to the flat corpora than to this one. The honest statement is not "unicode costs 5%" but "a stream that is three-quarters wide characters costs 5%, and everything else costs nothing."
What was recovered first
This was +6.2% before four optimizations went in: resolving a pending ZWJ with one character instead of handing the whole run to the per-character path (which is what actually moved it, 6.2% → 4.7%), repairing only the half the incoming character does not cover, reading the width at the call site so the ordinary case reaches no call, and asking for the wrap limit again only when a wrap moved the cursor. What remains is the arithmetic the invariant genuinely requires.
What accepting it should mean
Merging past the cumulative gate resets the baseline at the next tag, which turns this 5% into the new normal and hands the next change a fresh 5% on top of it. That is exactly the compounding the cumulative gate exists to catch, so the acceptance is worth making explicit rather than implicit:
- tag rc003 deliberately once this lands, rather than letting the baseline drift, and
- record in
CLAUDE.mdthatunicodecarries a known ~5% from the wide-cell invariant and why — so the next person to see it does not re-litigate it from scratch, and, more to the point, does not read it as slack to spend.
A two-column character occupies its own cell and the spacer after it. Nothing enforced that outside reflow, so six operations could leave half of one behind -- a width-2 cell whose second column now holds something else -- and a renderer meeting that draws a two-column glyph into one column and shifts the rest of the row. - Erasing (EL/ED/ECH) and shifting (ICH/DCH) cut wide characters in half. BufferLine.ReplaceCells has carried the repair all along and only reflow ever reached it; the repair now lives in Fill, which is where erasing goes, and widening the range there also gives the link, image and sized-run bookkeeping the true span that was cleared. - Printing over either half left the other. Both writers do the repair now: the run path in BufferLine.SetSingleWidthRun, beside the bookkeeping it already had to do for writing cells directly. - A wide character written at the last column was stored there with no room for its spacer, leaving the cursor past the pending-wrap position. The wrap test now knows the incoming character's width. Two clustering fixes in the same area: - GetStringCellWidth never clamped, so a cluster with two spacing marks measured 3 and disagreed with the incremental path about the same text, putting a width the cell machinery does not model into the buffer. - A ZWJ kept the cluster for whatever followed it. GB11 is ZWJ x Extended_Pictographic, so an emoji, a ZWJ and a letter used to swallow the letter into the emoji's cell where it could be neither seen nor selected. - The ASCII run path never consulted the pending ZWJ continuation, so the same bytes clustered differently depending only on how the host chunked them. It defers to Print while one is pending. Verified against the width corpora as well as the suite: NARROW 36,254/36,254, contested 187/187, the 25,021-grapheme language corpus. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wide-wrap test added a second translate-and-measure to every printed character, and the orphan repair added two array reads to every one: alt-redraw +32%, unicode +18%. Both answers were already available or cheaply gated. The width is now computed once, before the wrap decision, and reused by the write. The repair sits behind BufferLine.HasWideCells, a latch set when a two-column cell is written -- guarded at the CALL, which is the lesson this file keeps teaching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two array reads per run and per fill were measurable on the ASCII corpora, which never have a wide cell to orphan. One bool read now, shared by both writers through RepairAround. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ClusterTable's cap tested ConcurrentDictionary.Count, which takes every bucket lock and walks them -- on the print path, for every cluster. It reads the id counter now, which only overcounts by a lost GetOrAdd race, the safe direction for a cap. And the orphan repair read line[X].Width, where the indexer hands back a COPY of the cell struct; GetWidth reads the field. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…read The run-path repair was inside SetSingleWidthRun, where the extra call pushed that method past the JIT's inlining budget: scroll-ascii lost 8% to a lost inline, not to the one bool read it looked like. The guard belongs at the call, which is the lesson this file keeps teaching. The per-character repair read both neighbours; the cell being overwritten already says which half it is -- width 2 means its spacer is about to be orphaned, width 0 means it IS the spacer -- so one read decides both cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wide-character wrap test asked for it a second time, and it is not a field read -- it asks whether the cursor is inside the margin columns. On a CJK corpus, where the width-2 test passes constantly, that second call was most of what this bucket cost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…op the glyph that cannot fit Four review findings, two of them holes in this change's own invariant. HasWideCells was latched only by SetCell, but cells also arrive through the indexer, CopyCellsFrom, Clone and CopyFrom -- the bulk copies margin scrolling and reflow use, which write the array directly. A line that got its wide character that way kept a false latch, so every later repair skipped it and the orphan this change exists to prevent came back. The copy paths inherit the source's latch rather than inspecting what was copied: it is a latch, so over-approximating costs one repair that finds nothing, while under-approximating costs the invariant. CopyFrom assigns rather than ORs, because it REPLACES the contents -- a recycled scrollback line would otherwise carry the latch for the rest of the session. The early wrap that makes room for a two-column character is guarded by DECAWM, so with wrapping off the character reached the last column and was stored there while the margin refused its spacer: the same orphan, produced by the one path that skipped the guard. xterm.js parks the cursor at the last column and drops the character, and so does this now. The wrapping-off branch called WrapLimit() a second time, which is what the commit above it removed everywhere else. Reaching that branch means the early-wrap block did not run, so nothing has moved the cursor and the value already computed is the same one. The stray XML summary is removed; it belonged to SetCell and had detached onto HasWideCells.
The wide-cell repair runs on every printed character of every line that has ever held a wide character -- which is every line of CJK, where the latch is true from the first character on. Two pieces of work there are avoidable. The repair blanked the orphaned spacer at X+1 before writing, and then the wide character being written set X+1 as its own spacer immediately after, so CJK printed over CJK did that write twice and threw the first away. Only the half the incoming character does NOT cover can be orphaned: here == 0 always needs the character at X-1 blanked, here == 2 needs X+1 blanked only when what is landing is not itself two columns wide. The width under the cursor is read at the call site now rather than inside the helper. It is what decides whether there is any repair to make, so the ordinary case reaches no call at all, where before a true latch meant one call per printed character. The helper takes the value instead of re-reading it. The spacer's margin is computed once and read twice, for a wide character only -- ASCII never asks, so it never pays the call. NOT a measured win. Local single-run pairs showed unicode -3.7%, and CI's three-round measurement says the corpus is unchanged at +6.2%. The local readings for that corpus spanned 3.9% across builds that differ by nothing relevant, so the -3.7% was noise and the rule in CLAUDE.md -- a single pair proves nothing, CI is the arbiter -- was right. Kept because it is strictly less work either way: a write that is immediately overwritten, and a call per printed character, both go away. The 6.2% is still unaccounted for and wants a trace, not another guess.
The run path does not carry the pending ZWJ continuation, so a run arriving while one was pending was handed to Print a character at a time. But that state belongs to ONE character -- the one standing where the ZWJ was merged -- and the first Print clears it unconditionally. Every character after the first was paying the per-character path to be told about state that no longer existed, and text mixing emoji with words is ordinary rather than exotic. So only the first character goes through Print now, and the rest take the run. The other bailouts stay where they were: those are properties of the whole call, not of one character. The test that came with this had no coverage, and my first attempt at one was worthless -- it passed with the handling removed entirely. The letters never showed the bug, because GB11 continues a cluster only for a pictograph and a letter is not one. What survived was the STALE continuation, and the next pictograph printed at the column it still named was merged into the emoji from before the run: a woman printed at column 2 disappeared into the man's cell at column 0. The test drives that, and fails without either the original guard or this shortcut.
The spacer's margin was recomputed for every wide character, which in a corpus made of CJK is every character, to be told what the value taken at the top of the method already said. WrapLimit is a question about where the cursor IS, and between those two points the only thing that moves it to another row is the autowrap -- so the call now happens only when that actually fired, and ASCII never reaches it at all. Kept as a recomputation rather than a plain reuse. Forcing the stale value passes the whole suite, including a new test for a wide character at a DECSLRM right margin, so the two may well always agree -- but the early-wrap block moves the cursor past the margin before this point, and whether that can change the answer is exactly the kind of margin question this file has been wrong about before. One bool is not worth finding out the hard way. The margin test is new either way: nothing covered a two-column character landing on the right margin of a DECSLRM pane, which is the case that decides it.
e198665 to
3367138
Compare
Fifth of eight PRs from the whole-codebase audit. Stacked on #93 → #92 → #89.
This one has a perf cost I could not fully remove, and it exceeds the gate. Details at the bottom — please read that before merging.
The invariant
A two-column character occupies its own cell and the spacer after it. Nothing enforced that outside reflow, so six operations could leave half of one behind — a width-2 cell whose second column now holds something else. A renderer meeting that draws a two-column glyph into one column and the rest of the row shifts.
EL/ED/ECH) and shift (ICH/DCH)BufferLine.ReplaceCellshas carried the repair all along and only reflow ever reached it; it now lives inFill, and widening the range there also gives the link, image and sized-run bookkeeping the true span that was clearedPlus three clustering fixes in the same area:
GetStringCellWidthnever clamped (a cluster with two spacing marks measured 3 and disagreed with the incremental path); a ZWJ kept the cluster for whatever followed, where GB11 isZWJ × Extended_Pictographic, so an emoji + ZWJ + letter swallowed the letter into the emoji's cell; and the ASCII run path never consulted the pending ZWJ continuation, so identical bytes clustered differently depending only on how the host chunked them.Validation
8 new tests. Suite green: 1919. Width corpora unchanged: NARROW 36,254/36,254, contested 187/187, languages 25,021/25,021.
Perf — the part that needs your judgement
3×3 alternating vs main: scroll-ascii +0.8%, sgr-churn +1.7%, truecolor +2.2%, alt-redraw +0.3%, flood +0.7% — all inside noise. unicode +8.8% (±2%), over the 5% gate.
It started at +32% and I brought it down by isolating each cost with soak A/Bs and traces:
SetSingleWidthRun, where the extra call pushed that method past the JIT's inlining budget — scroll-ascii lost 8% to a lost inline, not to the bool read it looked like (−8% on ASCII)WrapLimit()is not a field read (it asks whether the cursor is inside the margin columns) and the new test called it a second time per character (−3% on CJK)ClusterTable's cap testedConcurrentDictionary.Count, which takes every bucket lock and walks them, on the print path — that one is mine from Bound what a program on the pty can make the terminal allocate or do #89 and is fixed hereTwo further attempts measured worse and were reverted: moving the wide-cell latch to the writers that know the width, and removing the latch entirely.
What remains looks inherent rather than accidental: the correctness fixes add real per-character work to the print path — a width-2 wrap test and an orphan check — and this corpus is deliberately CJK-heavy, so it pays that on nearly every character where real output would not.
Your call, and I'd rather you make it than have me keep grinding: take the 8.8% on a synthetic worst case for a correctness fix that affects every CJK and emoji user, or have me split the wide-cell repair into its own PR where the perf design can be the focus rather than a rider.
🤖 Generated with Claude Code