perf: cut terminal latency (interactive echo 16.4ms -> ~2ms, bulk queue wait ~5x)

[kshivang]

Why

BossTerm's benchmark suite says it beats iTerm2 and Alacritty on throughput (+43% vs iTerm2 at 1 MB), and that is true. It measures how fast the emulator consumes bytes: cat a file, time how long it takes to return.

It cannot see the interval that decides whether a terminal feels snappy - PTY arrival to the pixel that byte produces. An emulator can parse 1.6 GB/s and still wait 50 ms before drawing any of it, which is roughly what was happening.

The comparison that prompted this: the Rust terminal in BossConsoleRust reads 64 KiB, parses, and calls request_repaint(). No debounce, no timer, no frame cap. Coalescing is free because the frame clock does it.

Results

Measured with a probe added in the first commit, window raised, three runs per configuration.

	before	after
interactive echo (p50)	16.4 ms	~2.0 ms
bulk queue wait (p50)	786-1179 ms	147-262 ms
bulk queue wait (p95)	1572-1966 ms	262-360 ms
drawText per frame (tui)	208	30

Full numbers, method and the failed hypotheses: benchmark_results/LATENCY_BASELINE_2026-08-27.md.

What changed

Display path

• Removed the adaptive redraw debounce (8 ms per redraw, 50 ms once output passed 100 redraws/sec). Frame counts stay vsync-capped without it, so the sleep was never what prevented over-rendering - the frame clock was, plus the conflated channel and Compose's own per-frame coalescing. With the wait gone, RedrawMode, the rate detector and the mode-transition job had no purpose left: net -131 lines.
• performanceMode now defaults to latency rather than balanced, removing a 5 ms poll taken with the glyphs already in hand.
• Blanks extend a batched text run instead of flushing it, and ASCII cells skip the grapheme sequence probes.

PTY / parse path

• isAlive() is cached for 20 ms. UnixPtyProcess.exitValue() throws IllegalThreadStateException whenever the child is alive, and the drain loop called it once per character: ~5.5M exceptions with filled-in stack traces per 5 MB cat, 20% of parse time.
• Redraw requests are skipped when one is already queued. Each trySend to a parked consumer resumed it through the Swing dispatcher, meaning invokeLater + an InvocationEvent + an AccessController stack walk, per buffer mutation. Another 20%.
• ICU grapheme segmentation is skipped for printable ASCII (~27%).
• visualColToBufferCol short-circuits to the identity on lines needing no visual mapping. wrapLines called it with the full terminal width on every wrap (~10%, all from that one site).
• The debug collector no longer deep-copies the whole buffer on a 100 ms timer when nobody is watching. This is an idle-CPU fix, not a latency one - see below.

Verification

Automated: full suite green in both modules. The invariants that could fail silently are mutation-checked rather than trusted - the earliest-arrival rule, the ASCII guards, the ring-buffer counter, and the debug-panel gate each have a test that fails when the thing it protects is reverted.

Renderer changes fail by producing a wrong picture, which no unit test sees, so they were pixel-diffed: benchmark/latency/unicode-torture.sh rendered before and after, ZWJ families, flags, skin tones, variation selectors, CJK, powerline, underlines spanning gaps and aligned columns identical apart from subpixel antialiasing. Column alignment was checked specifically, since a merged run advancing by font metrics rather than cell width would drift progressively along a line.

By hand: no flicker under tui, bulk or vim; typing confirmed snappier; debug panel confirmed working.

Things worth a reviewer's attention

A fix that did nothing. Profiling first pointed at DebugDataCollector - 59% of samples. Gating it changed bulk latency not at all: that work runs on Dispatchers.IO workers parallel to the parse, and JavaMonitorEnter was zero, so it never blocked. A whole-JVM profile answers "where is CPU spent", not "what is on the critical path". The fix is kept because it removes 10 full-buffer deep copies per second per tab, but it is filed as idle-CPU, not latency.

A regression this branch shipped and then fixed. Gating the debug collector, I read debugEnabled (background collection) instead of debugPanelVisible (the UI, what Cmd+Shift+D toggles). The two fields are adjacent with near-identical names. Result: an empty debug panel, found on the first hand-check. The predicate is now a pure function with a test that reproduces the bug.

Bulk output is improved, not solved. Still ~150-260 ms on a 5 MB cat.

TerminalLine.merge is deliberately not fixed (~12% of remaining emulator time). writeCharacters rebuilds the whole line whenever a write is not at the end, and lines are NUL-filled to width so that is most writes. The rope-style entry walk is not a safe drop-in: collectFromBuffer coalesces adjacent runs by reference-equal style and an entry walk does not, so it would fragment lines permanently while charAt is O(entries) - a long-session regression a short benchmark cannot see. TerminalLineWriteModelTest is committed as the harness for whoever does it properly.

Also still open: residual AWT dispatch (~20% of non-parked emulator time) and joinTo/appendElement string building (~10%).

[claude]

Review: perf/terminal-latency (draft) — part 1/2

Strong framing, and the right instinct: the existing suite measures consumption, and "arrival → pixel" genuinely is the number that decides whether the terminal feels fast. The histogram design (log buckets, floor-reporting so it cannot manufacture a win), the mutation-checked invariants, and the honest caveat about draw-issued vs. photons are better discipline than most perf PRs get. Comments explain why rather than what, matching the house style.

Most of what follows is about probe fidelity rather than plumbing — since every later decision rests on these numbers, a systematic bias matters more here than in ordinary code.

---

1. pendingArrival is process-global, but there is one data stream per tab

FrameLatencyProbe is an object, so pendingArrival is shared across every BlockingTerminalDataStream in the process. BossTerm is a tabbed terminal, and background tabs keep streaming PTY output while only the foreground pane composes and paints.

Concretely: an AI CLI spinner in tab B stamps pendingArrival. Tab B never paints, so nothing consumes it. The next foreground paint in tab A — a cursor blink is enough — does pendingArrival.getAndSet(NONE) and records now - arrival, which can be seconds. Because markArrival is earliest-wins, that stale stamp also blocks every legitimate foreground arrival until some paint flushes it.

That lands squarely on p99 and max, i.e. exactly the numbers the README says to report and never to average away. It also means two measured builds can differ purely by what a background tab happened to be doing.

At minimum the harness should say "one tab, one pane, nothing else running" out loud. Better: key the pending slot per display (a FrameLatencyProbe instance handed to ComposeTerminalDisplay, with a shared default for the single-pane case), or have endFrame accept the stream identity it just drew.

2. markArrival fires before the bytes are in the buffer, so endFrame can consume a stamp the frame did not draw

took() marks arrival at the moment the emulator dequeues the chunk — before BossEmulator has parsed it into TerminalTextBuffer. The frame that later clears the stamp captured its snapshot at some earlier instant (ProperTerminal.kt:2058, under textBuffer.lock()).

So the ordering dequeue → stamp → [paint whose snapshot predates the parse] → endFrame records a too-small byteToPaintMs and discards the real sample. During bulk output, with paints firing continuously, that window is hit often. The kdoc claim — "arrival ... to the end of the paint pass that first draws it" — is not guaranteed by the mechanism, and unlike the vsync caveat this bias points downward, which is the direction that flatters a change.

Both this and item 1 have the same clean fix: read pendingArrival at snapshot-capture time inside the locked region in ProperTerminal, hand that value to endFrame, and only clear the slot if the snapshot actually included it. That is the point where "the buffer state this frame will draw" is well defined; endFrame is not.

Worth folding into the README caveat section too — it currently lists only the photons caveat, which reads as "a lower bound by one frame" rather than "arbitrarily low when a paint races a parse".

3. BOSSTERM_FAST_TEXT=1 changes pixels, not only cost

The commit message and README present the flag as a pure reduction ("Unset, the renderer behaves exactly as before" — true), but set, output changes for one case:

Previously a space could never join a batch (canBatch excluded it), and the per-character fallback at TerminalCanvasRenderer.kt:1129 draws nothing for a blank — so underlined blank cells rendered no underline at all. With blanks now extending a run whose batchIsUnderline matches, and visibleRunLength deliberately preserving full width when underlined, ESC[4m followed by ESC[K now paints the rule across the erased region.

That is almost certainly a fix and I would keep it. But it means the A/B is not a pure perf comparison and needs a visual check, and a leading underlined blank still draws nothing (it cannot start a run), so the behaviour is now inconsistent within a line. Worth stating in the commit message so the next reader does not treat a pixel diff as a regression.

4. visibleRunLength is applied unconditionally, outside the flag

TerminalCanvasRenderer.kt:898 runs the trim regardless of fastTextPath. It is a no-op today only because blanks cannot enter batchText when the flag is off — a non-local invariant two hundred lines away. Given that "no env set is a true baseline" is load-bearing for the whole methodology, either put the trim behind fastTextPath too, or leave a one-line note at :898 pointing at :1107 for why it is safe.

[claude]

Review: perf/terminal-latency (draft) — part 2/2

5. The ASCII guard soundness argument has one untested link

The skin-tone and regional-indicator halves check out against the code:

• checkRegionalIndicatorSequence requires the RI high surrogate at col itself — non-ASCII, caught at col.
• checkFollowingSkinTone can read as far as col+3 (TerminalCanvasRenderer.kt:1692 steps over a DWC), but the skin-tone high surrogate \uD83C must land at <= col+2, so col..col+2 is sufficient. CharUtils.DWC is , i.e. >= 0x80, so a DWC in the window also declines. Both fine.

The ZWJ half is different: containsZWJ(cleanText) scans up to 20 cells, so a ZWJ at col+3..col+19 genuinely flips hasZWJ in the old code while the 3-cell guard skips it. It is still equivalent — graphemes[0] is the leading ASCII char, grapheme.hasZWJ is false, and :1012 falls through to the normal path — but that second link is reasoning, not a test, and it is the one PlainAsciiFastPathTest does not pin. A corpus entry with "abc" + U+200D + "d" would cover it; the current "a<ZWJ>b" puts the ZWJ at col+1, inside the guard.

Relatedly, the corpus is built with CharBuffer(text), so no DWC markers are ever present. Both probes being validated have explicit DWC-interleaved layouts documented in checkRegionalIndicatorSequence kdoc, and the DWC step is precisely what pushes the skin-tone read window one past the guard. The test never exercises the buffer shape the real renderer sees. A couple of corpus entries with CharUtils.DWC inserted after each wide char would close that.

6. ~/.bossterm is hardcoded, bypassing BossTermPaths

FrameLatencyProbe.outFile builds ${user.home}/.bossterm/frame-probe.json directly. BossTermPaths.dir() (daemon/BossTermPaths.kt:32) is documented as the single source of truth and honours -Dbossterm.settings.dir specifically so multiple BossTerm-based apps on one machine stay independent — its kdoc calls out the two places that used to duplicate this. With the probe hardcoded, an embedder with a relocated store writes the snapshot to the shared dir, and two such apps would collide on frame-probe.json and consume each other frame-probe.reset marker.

7. enabled is a @Volatile var on the hottest path in the app

It is mutable only so the tests can flip it, but the cost is that the JIT cannot constant-fold the branch, so countDrawCall() pays a volatile load per drawText and the batching loop pays one per cell via the isPlainAsciiRun caller. Small in absolute terms — but it is overhead added to the very loop this PR exists to measure, which makes the baseline slightly not-the-baseline. An env-derived val plus an instantiable Histogram-only test surface would get the same coverage for free; most of FrameLatencyProbeTest already tests Histogram standalone and does not need the singleton at all.

That test-vs-singleton coupling is also an order-dependency risk: FrameLatencyProbeTest leaves enabled = true for the duration of each test and mutates shared histograms, so any other test in the module that renders (now or later, or under parallel forks) makes it flaky.

---

Smaller things

• workloads.sh name mismatch (will bite immediately). README.md says workloads.sh keystrokes; the script case arm is interactive. Copy-pasting from the README fails.
• probe.sh reset keys off 1-second-granularity mtime (stat -f %m / -c %Y). Correct but fragile. Since the sampler already writes JSON, having reset() bump a counter exposed as "resets": N and having the script wait for it to increment would be simpler and exact.
• FrameLatencyProbe.reset() does not clear BlockingTerminalDataStream.arrivalNanos. After a mid-workload reset, stamps enqueued before the reset are still dequeued and marked — the one contamination the reset exists to prevent.
• arrivalNanos ordering. The kdoc says "one per entry, in the same order", but arrivalNanos.offer and dataQueue.offer are not atomic together, so concurrent producers can swap pairings. Impact is negligible under earliest-wins; the claim should just be softened.
• drawCallsPerFrame percentiles are log-bucketed too (12.5% error), which the README field table does not mention. Fine for the order-of-magnitude claim, worth one line.
• Layering: terminal.BlockingTerminalDataStream now imports rendering.FrameLatencyProbe.
• Redundant charAt: isPlainAsciiRun(line, col, ...) re-reads line.charAt(col), which :971 already holds. More interesting: charAt (TerminalLine.kt:199) and getStyleAt (:584) are both O(number of text entries) linear scans, called per cell per frame. On a git diff --color row that is the dominant term in this loop, and walking the entries once per row would subsume both micro-optimisations here. Probably the thing to measure right after landing the probe.
• Dead branch: drawLength > 0 at :899 (and the visibleRunLength(" ") == 0 case) is unreachable from the renderer, since a blank can never start a batch. Harmless as a guard; the test asserts a state production cannot reach.
• Comment error: bucketsRoundTripWithinStatedError says "Four sub-buckets per octave puts the worst case at 1/8". The code uses eight (SUB_BITS = 3); four would be 1/4. The class kdoc has it right.
• TerminalCanvasRendererTestAccess says "File-private top-level functions are not reachable from a test", but isPlainAsciiRun is internal, and VisibleRunLengthTest in the same file calls visibleRunLength directly. The wrapper is dead weight and the comment is wrong.
• mkdir -p "$FIXTURE_DIR" runs before the case, so even a usage error creates the directory.

Test coverage gap worth closing before the flag becomes the default

visibleRunLength is well pinned, but the riskiest half of change 2 is not tested at all: that a blank joining a run with different fg/bold/italic still leaves batchStartCol and the run text aligned. Runs are much longer now, so an origin shift or a dropped cell would smear a whole line rather than a word — the one failure mode a user would actually notice. Extracting the batch decision into something that returns the (startCol, text, style) runs for a row, and asserting against a hand-written expectation for a few rows (mixed colours around blanks, underline transitions, a double-width char mid-run, a hidden cell mid-run), would cover it without needing to render.

Verification note

Reviewed statically; I did not build or run anything, per AGENTS.md. Findings are read from the code paths cited; items 1, 2 and 3 are the ones I would want confirmed empirically before the measurements are used to pick defaults.

Nice piece of work overall — the pushback is all "calibrate the instrument before trusting its readings", which is the failure mode this PR is otherwise unusually careful about.

[claude]

Review (1/2) — correctness and measurement validity

I read the full diff plus the surrounding renderer and data-stream code. This is unusually good work: measurement before optimisation, the two risky invariants mutation-checked rather than asserted, floor-reporting quantiles so the instrument cannot manufacture a win, and finding 3 arguing against the author's own next step. The HIGH_VOLUME debounce result alone justifies the PR.

I could not run ./gradlew build in this environment, so all of the below is static analysis.

1. The ASCII fast path still pays the per-cell ThreadLocal and allocation it set out to remove

TerminalCanvasRenderer.kt:984-1001. plainAscii only gates the fill loop:

val builder = zwjCheckBuilder.get()   // still runs
builder.setLength(0)                  // still runs
run { while (!plainAscii && …) { … } }
val cleanText = builder.toString()    // still runs -> fresh empty String, every cell

StringBuilder.toString() with count == 0 goes through StringLatin1.newString -> new String(…); it is not interned, so that is a live allocation per cell per frame. Meanwhile isPlainAsciiRun adds three charAt calls. So the fast path removes the 20-char scan and the three probes but keeps a ThreadLocal.get(), a setLength and an allocation, and adds work on top.

That is a plausible cause of the two paintCost regressions in the baseline table (interactive 1.15 -> 1.79, bulk 1.41 -> 2.56, fast-text only) — short-line workloads where the eliminated scan was cheap anyway. Hoisting the whole block into if (!plainAscii) { … } is a couple of lines and worth re-measuring before any conclusion rests on those rows.

For what it's worth the soundness argument checks out, and more robustly than the KDoc claims. checkFollowingSkinTone can read as far as col + 3 (checkCol = col+1, +1 past a DWC, then charAt(checkCol + 1)), outside the documented col..col+2 window — but it only reaches there if c1 at checkCol ∈ {col+1, col+2} equals '\uD83C', which the guard rejects first. And CharUtils.DWC is , itself >= 0x80, so a DWC in the window independently forces the slow path. The window is correct for a second reason beyond the one written down.

2. bulk fast-text-only contradicts its own mechanism, and the prose reads past it

benchmark_results/LATENCY_BASELINE_2026-08-27.md:41-42: with fast-text on, drawCallsPerFrame p50 goes 20 -> 56 and byteToPaint p50 goes 81.9 -> 589.8. Blank-merging can only ever reduce draw calls for given frame content, so either the sampled frames differ substantially (likely — at 589 ms p50 you sample far fewer, much larger frames) or something in the fast path misbehaves on that workload. The text reads this row as "does not help"; the table's honest reading is "regresses two of four workloads on the metric it targets".

That is the row that would gate making BOSSTERM_FAST_TEXT default, so I would re-run it with per-cell n. No cell currently carries n, a trial count, or any spread, which sits oddly beside the document's own (correct) "report percentiles, never means" discipline.

3. probe.sh reset can print "reset confirmed" without a reset

benchmark/latency/probe.sh:36,43:

before=$(stat -f %m "$OUT" 2>/dev/null || stat -c %Y "$OUT")

On GNU coreutils stat -f means --file-system and does not accept -c-style formats, so %m is parsed as a filename. Verified on Linux: it errors on %m (exit 1, suppressed) but still prints a full filesystem report to stdout first, and the || fallback then appends the mtime. So before and now are multi-line strings containing Blocks: Total/Free/Available.

Those counts drift on any busy machine, so [[ "$now" != "$before" ]] can be true while the snapshot was never rewritten — the loop exits 0 and the workload starts against counters still holding the previous run. That is exactly the failure the wait loop exists to prevent, and it fails silently toward a contaminated measurement.

Reverse the order (stat -c %Y "$OUT" 2>/dev/null || stat -f %m "$OUT"), or better, drop mtime and poll the snapshot's own uptimeSeconds / "n": 0 — the JSON already carries everything needed to confirm a zeroed histogram.

4. benchmark/latency/README.md:48 documents a subcommand that does not exist

workloads.sh keystrokes — the script's arms are interactive|bulk|tui|scroll|aged, so the first command in the documented sequence exits 1. LATENCY_BASELINE_2026-08-27.md:106 has it right.

5. Shutdown hook and sampler race on the same .tmp path

FrameLatencyProbe.kt:111 and :120 both call writeSnapshotQuietly(), from frame-probe-final and frame-probe-sampler, and both stage to outFile.name + ".tmp" before renaming. A shutdown landing mid-sample can interleave the two writeText calls and leave a truncated or doubled JSON as the final artefact — the one a run is read from. synchronized, or a thread-distinct tmp name, closes it. The atomic-rename instinct is right; it just is not sufficient with two writers sharing one staging path.

6. markArrival fires before the byte is drawable

BlockingTerminalDataStream.took() stamps on the emulator thread at take time, before the chunk is appended to buffer and parsed. A paint inside that window (a blink toggle, or the paint for an earlier trigger) consumes pendingArrival and records a byteToPaint for a frame that does not contain the byte. That biases downward, on top of the GPU-present bound the docs already own up to. On interactive, where the whole figure is 16.4 ms, one stolen frame is a ~100% error. Either stamp after the parse, or extend the honest-caveat section — it currently attributes the lower-bound property solely to present/vsync.

7. The probe is process-global; the app has N panes and tabs

pendingArrival, pendingTrigger and drawCalls are singleton state, but renderTerminal runs once per canvas. With a second pane or tab painting, an arrival destined for tab A can be consumed by tab B's paint, and beginFrame's drawCalls.set(0) becomes per-canvas rather than per-frame. Neither doc states "single tab, single pane" as a precondition — document it, or key the state per-canvas.

Related: FrameLatencyProbe.report() (:232-244) and the README field table were never updated for triggerToPaintMs, which json() does emit and which baseline findings 1 and 4 rest on entirely. A reader of report() will not know the series exists.

[claude]

Review (2/2) — code quality, security, test coverage

Smaller code-quality points

• styleMatches is not gated on fastTextPath (TerminalCanvasRenderer.kt:1109-1113) while canBatch is. Currently harmless: with the flag off, canBatch is false for any blank so the else-branch is taken regardless. But it means the "unset behaves exactly as shipped" guarantee depends on a second predicate to neutralise this one. (fastTextPath && isBlankCell) || … would keep that guarantee local to one expression rather than spread across two.
• TerminalCanvasRendererTestAccess is dead weight and its comment is factually wrong. isPlainAsciiRun is internal, not file-private, and VisibleRunLengthTest two classes down in the same file calls visibleRunLength directly — which proves internal top-level functions are reachable from desktopTest. Drop the wrapper.
• Stale comment, FrameLatencyProbeTest.kt:33: "Four sub-buckets per octave puts the worst case at 1/8 of the value." The implementation uses eight (SUB_BITS = 3), and the production KDoc correctly notes that four would be 1/4. The test's own bound (v / 8 + 1) is right; only the comment is wrong. Worth fixing in a PR whose whole premise is measurement rigor.
• visibleRunLength's all-blank case is unreachable. A blank can never start a batch, so batchText[0] is always a glyph and drawLength >= 1 always holds. Fine as defensiveness, but the KDoc asserts "a run that is nothing but blanks need not be drawn at all" as a real case, and trailingBlanksAreNotShaped pins " " -> 0, a state the renderer cannot produce.
• arrivalNanos boxing and single-producer assumption. ConcurrentLinkedQueue<Long> boxes a Long and allocates a node per chunk. Only when probing, so acceptable — but FrameLatencyProbe's "nothing in this file allocates on the hot path" does not extend to the data-stream side, and the reader may carry it over. Separately, enqueue/took keep the two queues aligned only under a single producer; append() is not synchronized (pre-existing), so a second producer could interleave the two offers and pair a chunk with the wrong stamp. The /** … Keeps the two queues aligned. */ comment claims more than the code guarantees.
• enabled is @Volatile, so countDrawCall()'s guard is a real memory load per drawText that the JIT cannot fold away — unlike fastTextPath, which is a plain val and does fold. Negligible at ~200 calls/frame, but if the "inert when off" claim should be literally free, a non-volatile val seeded from env plus a separate test-only override gets there.
• The debounce override plumbing reads well, and delay(0) genuinely returns without suspending (if (timeMillis <= 0) return), so the comment at ComposeTerminalDisplay.kt:55 is accurate.

Performance

No concerns with the instrumentation cost when off — one volatile read at each of five call sites, and max.accumulateAndGet takes a non-capturing lambda, so Kotlin compiles it to a singleton LongBinaryOperator with no per-record allocation. String.format in json() runs once a second, off any hot path. The histogram round-trip is correct: I checked indexOf/valueOf at octave boundaries (8, 15, 16, 17) and the >= BUCKETS clamp, and the floor-reporting property holds.

Security

Nothing concerning. BOSSTERM_FRAME_PROBE_OUT is an unvalidated write path — the app will mkdirs() its parent and write a .tmp sibling plus a frame-probe.reset sibling (which it then deletes) wherever it points — but it is env-gated and dev-only, no worse than the flag it rides on. More importantly the payload is aggregate timings only: no PTY bytes, no titles, no cwd, nothing that would make ~/.bossterm/frame-probe.json sensitive. That is the right call and worth preserving if the probe ever survives to ship.

Test coverage

Good where it counts. The two mutation-checked invariants are the right two, and fastPathStillFiresOnOrdinaryText guarding against a fast path that never engages is the kind of thing most PRs skip. Three gaps:

1. Nothing covers enqueue/took pairing in BlockingTerminalDataStream — the one place two-queue alignment can drift, with four distinct paths (close sentinel, closed non-blocking poll, null poll, normal take). Append three chunks, drain, assert three arrivals in order.
2. Nothing pins the flag's off-state. "Unset, the renderer behaves exactly as before" is the PR's central safety claim and is currently reasoning only. fastTextPath is a file-private val read at class load, so it is not injectable; making it settable the way FrameLatencyProbe.enabled is would let one test assert that a blank still breaks the batch with the flag off.
3. The batching decision is untested. visibleRunLength and isPlainAsciiRun are covered, but canBatch/styleMatches is where the interesting rules live: a blank inherits the run's colour and weight but must match underline; a blank may join a run but not start one. I traced these by hand and believe they are correct — including that [ul "a"][ul " "][no-ul "b"] now correctly extends the rule across the space, an improvement on the previous behaviour where an underlined space got no rule — but that is a lot of reasoning riding on one inline boolean. Extracting it as a predicate would make it pinnable.

Also worth noting: FrameLatencyProbeTest mutates a process-global singleton's enabled flag and shared histograms. setUp/tearDown handle it correctly today, and since the env flag is unset in CI the sampler thread never starts and no files are written — but it is a latent ordering hazard if test parallelism inside a JVM is ever enabled.

---

Draft status noted, and scaffolding-with-a-stated-exit-plan is the right approach for this. The three I would fix before the next measurement round — because they affect the numbers rather than the code — are #1 (the fast path is not as fast as intended), #3 (reset can silently no-op) and #2 (re-run bulk with n reported).

[claude]

Review (1/3) — summary and the things I'd resolve before merge

Read the full diff plus surrounding code in TerminalCanvasRenderer, BlockingTerminalDataStream, ComposeTerminalDisplay, SettingsManager and PerformanceSettingsSection. I did not build or run anything (no gradle in this environment), so everything below is from reading.

What's genuinely good and worth keeping in this shape:

• Measurement before tuning, with the probe split at the redraw trigger. byteToPaint vs triggerToPaint is what turns "it feels slow" into "485 ms of it is upstream of the trigger", and that split is why the bulk conclusion is actionable instead of a guess.
• Finding 0 (occluded window throttled to ~3 fps) is the most valuable thing here for anyone who measures this again. Documenting a 30x artefact that looks like a product bug is exactly right.
• The self-correction on the "1310 -> 10.2 ms" claim, in the file, naming the reason (33 samples).
• Histogram design: no allocation on the paint path, bounded memory, quantiles report the bucket floor so the bias runs against manufacturing an improvement. Invariants mutation-checked rather than asserted in prose.
• visibleRunLength extracted precisely because it's the one place a character could be dropped. Right instinct.

1. The performanceMode default change reaches no existing user

SettingsManager.kt:101 sets encodeDefaults = true ("Ensure all fields are written, not just non-default ones"). So every existing ~/.bossterm/settings.json already contains a literal "performanceMode": "balanced", and changing the Kotlin default at TerminalSettings.kt:636 only affects fresh installs.

The 16.4 -> 12.3 ms slice of the headline win is therefore invisible to the installed base. This wants a one-time migration on load (stored "balanced" -> "latency"), or the results doc should say plainly that the row applies to new installs only.

2. Two p50 regressions sit in your own results table and are never discussed

workload	baseline p50	shipped defaults p50
tui	9.2	13.3 (+45%)
bulk	81.9	491.5 (6x)

tui paintCost also moves 11.26 -> 12.29. The doc handles the bulk p95 correction with real rigour, then "What shipped, and what it bought" discusses only the interactive win. The TUI/spinner regime is the one the PR description leads with ("the regime of any TUI or AI CLI spinner") — a 45% p50 regression there deserves a paragraph, even if the conclusion is "p95 unchanged at 15.4, p50 under one frame either way, accepted".

Relatedly, the one config that helps tui — fast-text only: p50 8.2, paintCost 9.22, 28 draw calls — is the one that ships disabled, and shipped defaults + BOSSTERM_FAST_TEXT=1 is the missing cell in the matrix. That's the config you'd actually want to ship, and it was never measured.

3. Missing measurement: keystroke echo during bulk/tui output

Every row is one workload in isolation. The HIGH_VOLUME throttle's real job was arguably to stop the Main dispatcher being hammered with one coroutine resumption + one Compose snapshot write per PTY chunk. With it gone, redrawChannel almost never actually conflates (the processor drains it immediately, since nothing sleeps), so a 5 MB cat puts ~640 dispatches on the same thread as the frame clock. Whether that starves an interleaved keystroke echo is the regression this change is most likely to have, and no workload combines the two.

4. BOSSTERM_FAST_TEXT should not ship as an env gate

The PR text says the flag comes out once there are numbers — and LATENCY_BASELINE_2026-08-27.md has the numbers. Shipping it off leaves two divergent text-rendering paths in the production binary where only one gets user exposure, and leaves PlainAsciiFastPathTest guarding code no user executes. Either flip it on (strictly better on tui) or land the renderer reductions separately. Same for the "Not ready to merge" framing: the description still lists BOSSTERM_REDRAW_DEBOUNCE_MS / BOSSTERM_HIGH_VOLUME_DEBOUNCE_MS, neither of which exists in the diff, and the debounce is now deleted outright rather than A/B-able.

5. benchmark/latency/README.md documents things that don't exist

Anyone following it hits a usage error on the first command:

• BOSSTERM_REDRAW_DEBOUNCE_MS and BOSSTERM_HIGH_VOLUME_DEBOUNCE_MS (README:68-69, 78, 86-87) are not in the code — rg finds them only in markdown. The whole "Suggested ladder" section describes a knob set that isn't there.
• workloads.sh keystrokes (README:48) — the script only accepts interactive|bulk|tui|scroll|aged.
• README:71 lists the performanceMode baseline as balanced; it's now latency.
• README:48 calls interactive "200 single keypresses at a prompt". The script's own header is honest about this (printf 'x' is PTY output, and no workload covers the input half) — the README should be too.
• The field table (README:20-26) omits triggerToPaintMs, the most useful of the five. FrameLatencyProbe.report() omits it too, while json() includes it.

[claude]

Review (2/3) — measurement honesty, and the renderer change

6. Probe attribution: the arrival stamp is consumed at dequeue, not at render

took() (BlockingTerminalDataStream.kt:83-89) calls markArrival the moment a chunk leaves dataQueue — before it is parsed into the buffer. If a paint lands between dequeue and the model update, endFrame consumes that pendingArrival and records a byteToPaint sample for a frame that did not draw the chunk; the frame that actually draws it then gets no sample at all.

On interactive (one chunk, 50 ms apart) this is harmless. On bulk, where the emulator parses continuously, it biases the number downward — so 491.5 ms is likely an under-estimate, for a reason unrelated to the stated draw-issued caveat. Either document it alongside that caveat, or move stamp consumption to the redraw-trigger boundary where the data is known to be in the buffer.

7. lockedCaptureMs measures more than it claims

ProperTerminal.kt:2058 takes the start timestamp before textBuffer.lock() (line 2059), so it includes lock acquisition wait. The KDoc (FrameLatencyProbe.kt:26, :202-206) and README:24 both say "time holding the lock". Including acquisition is arguably the more useful number (it's the real UI-thread stall), but as written it silently conflates contention with capture cost — which matters for the aged conclusion, since contention and snapshot depth would both push it up.

8. Underlined blanks inherit the previous run's foreground colour (fast-text on)

styleMatches (TerminalCanvasRenderer.kt:1109-1113) skips the fg/bold/italic checks for a blank, and flushBatch draws the underline in batchFgColor (:913) across batchText.length (:911). So ESC[4;31mabc followed by ESC[4;34m draws a red rule under the blue-underlined blanks.

Narrow — and note the flip side is a real fix: before this change a blank always took the else branch and was skipped entirely, so an underlined space drew no rule at all (ESC[4m over a b underlined the letters, not the gap). Worth saying so in the comment, since it's a behaviour improvement hiding inside a perf change.

9. TerminalSettings KDoc numbers are wrong for one of the two call sites

The new KDoc says throughput waits "up to 10 ms" and balanced "up to 5 ms". True of readNonControlCharacters (BlockingTerminalDataStream.kt:316-322), but the main read loop (:256-264) uses 100 ms / 10 ms. Since the point of the rewritten comment is to tell a reader what they're paying, the larger number is the one they want.

10. Settings UI copy now contradicts the code

PerformanceSettingsSection.kt:187 still lists "Balanced" first, and :195 still describes it as "Good for most users. Balances quick response with efficient bulk output handling." The new KDoc says the same mode is pure added latency on an interactive echo. A user opening Settings is told to pick the thing you just measured as worse.

---

Renderer: worth eyeballing before this goes on by default

Full-width runs are now the norm, not the exception. A batched run used to be word-length; merging blanks makes it line-length. Two latent things become the common path:

1. drawText(textMeasurer, …) lays out with maxWidth = canvasWidth - topLeft.x and softWrap defaulting to true. A ~200-cell run measuring a hair wider than the canvas will wrap and paint a second line of text over the row below. Cheap hardening: pass softWrap = false, maxLines = 1 in drawTextClipped.
2. Pass 1 positions backgrounds with per-cell floor/ceil (:796, :819) while pass 2 positions text at raw visualCol * cellWidth (:1024) and lets the shaper advance the rest of the run. If the font advance and ctx.cellWidth disagree by a fraction of a pixel, drift used to be bounded by word length and is now bounded by line length. Worth a look at column ~200 with a fractional cellWidth.

Neither is strictly new — a space-free line like a ==== divider already produced a full-width run — which is why I'd call this "check it" rather than "bug".

isPlainAsciiRun re-reads each cell ~3x. Three charAt per cell across the line. Replacing the 20-char scan with a 3-char scan is already the big win; a single per-line pass recording the index of the last non-ASCII char (or just a lineIsPureAscii flag) would make the common case one check per line.

I did verify the guard's soundness argument holds: CharUtils.DWC is U+E000 (CharUtils.kt:21), so the DWC-skip branches in checkFollowingSkinTone can't be reached with col..col+1 ASCII, and checkRegionalIndicatorSequence requires a non-ASCII high surrogate at col itself. containsZWJ over the 20-char lookahead can only change the outcome via graphemes[0].hasZWJ, which needs a ZWJ at col+1. The col..col+2 window has a cell of margin.

[claude]

Review (3/3) — test coverage, small stuff, security

Tests

The three mutation-checked invariants are the right ones. Gaps:

• The ASCII corpus has no DWC markers. terminalLine() builds CharBuffer(text) directly, so the wide/emoji entries never carry the U+E000 the emulator actually inserts after a wide char. The guard's soundness rests on DWC being non-ASCII, and both probes have explicit DWC-interleaved branches (checkRegionalIndicatorSequence's KDoc documents three distinct layouts) that the test never reaches. Add the DWC-interleaved forms.
• Nothing covers the blank-batching change end-to-end. visibleRunLength is pinned, but the interesting invariants live in canBatch/styleMatches: a blank may not start a run; a blank must not extend a run whose underline state differs; a colour change after interior blanks must flush. Extractable the same way visibleRunLength was — and that's where finding 8 lives.
• Nothing pins the arrivalNanos / dataQueue alignment, the one place the probe touches a hot production path. A test pushing N chunks through and asserting arrivalNanos drains to empty (including a CLOSE_SENTINEL pass-through, which deliberately consumes no stamp) would be cheap. The alignment breaks if enabled flips mid-stream — production never does, but FrameLatencyProbeTest does, so it's a real if unlikely cross-test hazard.
• FrameLatencyProbeTest mutates a process-wide singleton and asserts exact counts on the shared byteToPaint / paintCost. Safe today (no maxParallelForks anywhere in the build), but a landmine the day test parallelism is enabled. Histogram is already instantiable; only the two frame-level tests need the object.
• bucketsRoundTripWithinStatedError's comment says "Four sub-buckets per octave puts the worst case at 1/8". SUB_BITS = 3 is eight sub-buckets; the class KDoc gets it right (eight -> 1/8, four -> 1/4). The comment contradicts the code it exists to pin.
• TerminalCanvasRendererTestAccess says "File-private top-level functions are not reachable from a test" — but isPlainAsciiRun is internal, and VisibleRunLengthTest two classes down calls the internal visibleRunLength directly, which proves it. The wrapper and its comment can both go.

Small stuff

• RedrawRequest is now a data class whose only property is never read, and System.currentTimeMillis() runs per redraw request for nothing. Channel<Unit> drops an allocation and a clock read per PTY chunk — on the exact path this PR optimises. Also a stray trailing comma after the last parameter.
• Stale KDoc in ComposeTerminalDisplay: requestRedraw still says "normal priority, applies debouncing" (:356) and "which is the intended debouncing behaviour" (:369); requestImmediateRedraw still says "bypasses debouncing" (:438). The class doc was rewritten thoroughly, so these read as misses rather than intent.
• requestImmediateRedraw still bypasses the conflated channel entirely (one coroutine launch per call, no coalescing). With the debounce gone that's now the only difference between the two entry points, and it's the unbounded one. All 20-odd callers are UI-driven so the rate is low — but the two could probably collapse into one.
• FrameLatencyProbe.enabled being a mutable @Volatile var (for test access) prevents the JIT folding the guard away. A val from env plus a separate test seam would make the disabled path genuinely free. Almost certainly immaterial — but countDrawCall is per-drawText.
• Removing the public nested RedrawMode / RedrawPriority is source-breaking for embedders. No in-repo users, and AGENTS.md says no back-compat hacks, so fine — just worth a heads-up for terminal-tab/BossConsole.

Security

Nothing of concern. BOSSTERM_FRAME_PROBE_OUT lets a user redirect the snapshot to an arbitrary path and writeSnapshotQuietly will mkdirs() the parent, but that's user-controlled env acting on the user's own behalf and the payload is timing histograms — no voice.json-style chmod treatment needed. The probe being inert and adding zero product surface without the flag (file-based reset marker rather than an MCP tool) is the right call.

---

Verdict. The measurement infrastructure and the interactive result are solid work and I'd land them. What I'd want resolved first: the encodeDefaults migration gap (1), the undiscussed tui/bulk p50 regressions plus the unmeasured defaults + fast-text cell (2), the fast-text env gate (4), and the README/description drift (5). Findings 6 and 7 are measurement-honesty items, and they matter because the next round of work will be scoped off these numbers.

[claude]

Review (1/3): summary and the four things I would resolve first

Strong work. Measuring before tuning, splitting byteToPaint at the redraw trigger so "debounce" and "slow recomposition" cannot be confused, retracting the bogus 99% bulk claim in the same file that made it, and mutation-checking the invariants that could fail silently - that is the right shape for a perf change. PlainAsciiFastPathTest's anti-vacuity check (assert the guard engages, so a never-firing guard cannot pass trivially) is what makes the test set worth something.

I checked the ASCII guard independently and it holds: CharUtils.DWC is U+E000 (>= 0x80), so the one path where checkFollowingSkinTone reads as far as col+3 - a DWC at col+1 - is already refused by the col..col+2 scan. The "distant ZWJ" case that worried me (ASCII prefix, ZWJ emoji 8 cells later, old lookahead sets hasZWJ at col 0) is behaviour-preserving, because the inner graphemes[0].hasZWJ gate rejected it anyway. CI is green on all three platforms; I could not run a build in this environment.

1. The PR description no longer matches the diff

The body says the renderer work is "behind BOSSTERM_FAST_TEXT=1", that three A/B env vars exist, and that "with none set the build behaves exactly as shipped". None of those three strings exist anywhere in the tree - all three changes are unconditional defaults, which the baseline file and the new ComposeTerminalDisplay KDoc correctly describe. The flags were the whole argument for why this was safe as a draft, so the body should be rewritten: this is a defaults change to the render and PTY paths.

2. The performanceMode default change reaches almost nobody

SettingsManager.kt:101 sets encodeDefaults = true, deliberately. Every existing ~/.bossterm/settings.json therefore already contains "performanceMode": "balanced" verbatim, so flipping the Kotlin default only affects installs that never saved settings. The measured ~4 ms is invisible to the existing user base without a one-shot migration (balanced -> latency on load, keyed off a settings version). Either add that or say fresh-installs-only, so the number is not over-claimed.

Docs still carry the old default: README.md:345, docs/wiki/Configuration.md:33 and :91, docs/wiki/API-Reference.md:303. And PerformanceSettingsSection.kt:186-196 still lists "Balanced" first with "Good for most users".

3. The underline-across-blanks change is real, and the report says it is not

The most important one, because the pixel diff is the only verification the renderer change has.

Old code: a space could neither batch (char != ' ' in canBatch) nor draw on the fallback path (the char != ' ' && char != NUL guard before renderCharacter). So an underlined space rendered nothing at all - including no underline rule. ESC[4munderlined with gaps drew three separate underlines with bare gaps between them. Now blanks extend the run and flushBatch draws one continuous rule across batchText.length * cellWidth.

That is a fix (xterm/iTerm2 behaviour). But benchmark_results/LATENCY_BASELINE_2026-08-27.md asserts "underlines spanning gaps ... are pixel-identical apart from subpixel antialiasing (0.31% of pixels ...)". Both cannot be true. Either the fixture's underline row was not in the compared region, or 0.31% swallowed a real change. Worth re-running that row specifically - if the diff missed a visible, intended change, it is weaker evidence for the no-drift claim than the file presents it as.

4. Merged blanks take the run's underline colour, not their own

styleMatches now ignores fgColor for blanks, but flushBatch draws the underline with batchFgColor. ESC[4;31mred then ESC[4;32m green: the space is green and underlined, merges into the red run, and its rule paints red. Better than the old "no rule at all", so not a regression - but a known-wrong pixel, and cheap to close: when batchIsUnderline is true, keep requiring batchFgColor == fgColor even for blanks. Non-underlined blanks, which are what actually pays for the optimisation, keep merging.

[claude]

Review (2/3): probe fidelity, and hygiene

5. The probe is process-global; arrivals are per-stream

pendingArrival, pendingTrigger, drawCalls and the histograms all live on a singleton, but every tab and every split pane has its own BlockingTerminalDataStream (TabController.kt:512, 733, 901, 1153) and its own Canvas calling beginFrame/endFrame. With two or more panes live:

• pane A's paint calls pendingArrival.getAndSet(NONE) and consumes a chunk that arrived on pane B's stream - crediting A a latency it never paid, and discarding B's real sample;
• a background tab that never paints still stamps arrivals into the shared slot, which the focused pane's next paint eats.

Both bias byteToPaint down, and neither benchmark/latency/README.md nor the baseline file records how many tabs were open. Cheapest fix is a documented single-tab, single-pane precondition; the sound fix is per-stream pending slots.

Smaller, same class: the cursor overlay is a separate Canvas, and its drawTextClipped calls hit countDrawCall() outside any beginFrame/endFrame pair, so those increments land in whichever frame happens to be open.

6. endFrame can credit a frame with a chunk it never drew

recordSnapshot / createIncrementalSnapshot runs during composition (ProperTerminal.kt:2058-2072); endFrame runs at the end of the draw pass. A chunk taken between those two points sets pendingArrival, and endFrame consumes it - a near-zero latency attributed to a frame whose snapshot predates the chunk, and that chunk's real sample lost for good.

Under bulk output that window is exactly when chunks are arriving, so it biases byteToPaint down right where the most surprising number in the report sits (bulk p50 491 ms). Capturing the pending arrival at snapshot time and threading it into endFrame closes it, and makes the triggerToPaint split - the best idea in this file - trustworthy at the tail too.

7. enqueue() is not atomic across the two queues

Two offer calls with no lock, so interleaving producers pair a stamp with the wrong chunk. Single-producer per stream today as far as I can tell, and the skew is small for a diagnostic - but the KDoc claims the queues stay "in the same order", so the single-producer assumption belongs written down next to that claim. (CLOSE_SENTINEL at :224 correctly bypasses enqueue and took() correctly skips it - that part is careful.)

Nits

• Stale debounce comments in ComposeTerminalDisplay: the ADAPTIVE DEBOUNCING LOGIC banner (:302), "handles debouncing" (:306), requestRedraw's "applies debouncing" (:354), "the intended debouncing behaviour" (:368), and requestImmediateRedraw's "bypasses debouncing" (:439). In a file whose new header explains at length that nothing waits on a clock, these will mislead the next reader.
• RedrawRequest is now a token with an unread field. timestamp is written by every caller and read by nobody, so each request allocates a data class to carry nothing. Channel<Unit> says what it is now.
• The new performanceMode KDoc is half right: it says throughput waits 10 ms and balanced 5 ms, but there are two poll sites - BlockingTerminalDataStream.kt:262-264 is 100 ms / 10 ms, :320-322 is 10 ms / 5 ms. The 100 ms one is the bigger number and goes unmentioned.
• benchmark/latency/README.md:54 documents workloads.sh keystrokes; the script's case label and usage string are both interactive.
• FrameLatencyProbeTest.kt:438 contradicts the code it pins: "Four sub-buckets per octave puts the worst case at 1/8". SUB_BITS = 3 is eight sub-buckets - the Histogram class doc gets this exactly right (and explains why four would be 1/4), so the test comment is the odd one out.
• visibleRunLength's all-blank case is unreachable. canBatch forbids a blank from starting a run, so batchText[0] is never blank and drawLength >= 1 always; the if (drawLength > 0) guard and "a run that is nothing but blanks need not be drawn at all" describe a state the batcher prevents. The trim is load-bearing for a different reason worth documenting: cells past line.length() read as CharUtils.EMPTY_CHAR = ' ' with a null style, so every batch now extends to the full visible width, and the trim is what stops each short line from shaping 80 cells.
• Histogram.reset() is field-by-field, called from the sampler thread while the UI thread records, so a reset can interleave into a torn state (counts zeroed after total was bumped) and mean is briefly nonsense. Fine for a diagnostic - worth one line, given how carefully the rest of the class reasons about its own error bounds.
• "Nothing in this file allocates on the hot path" has a hidden dependency: max.accumulateAndGet(v) { a, b -> ... } is allocation-free only because indy SAM conversion caches the non-capturing lambda. A plain CAS loop makes it unconditionally true.
• isPlainAsciiRun reads each cell three times (every column scans col..col+2), and TerminalLine.charAt walks myTextEntries, so a truecolour line pays that walk 3x per cell. Still a large net win against 20 charAt calls plus a String plus containsZWJ, but a single per-line scan for the first non-ASCII column, kept as a high-water mark, would be strictly cheaper.
• maxRefreshRate has no consumer - declared at TerminalSettings.kt:641, merged at TerminalSettingsOverride.kt:250, driven by a slider at PerformanceSettingsSection.kt:206, and read nowhere in the tree. Pre-existing, but this PR removes the last redraw throttle, so it is the natural moment to wire it up or delete it; right now a user who sets 30 fps to save battery gets nothing.

[claude]

Review (3/3): test coverage, security, performance

Test coverage

What is here is well-aimed: bucket round-trip against a stated error bound, a known distribution to catch the classic inverted-percentile bug, empty-histogram-is-not-zero-latency, earliest-arrival-wins, idle frames excluded, per-frame draw-call reset, the disabled path, visibleRunLength including the interior-blank case, and the ASCII guard checked against the real probes at every column over a proper corpus plus its anti-vacuity twin.

Three gaps:

1. The batching decision itself is untested. The rules that actually changed - a blank may not start a run, a blank ignores fg/bold/italic but not underline, NUL becomes a space - sit inline in a ~250-line loop and are covered only by the manual pixel diff. Extracting a small pure blankMayExtend(...) (or RunState.canExtend(...)) would pin findings 3 and 4 as tests rather than as prose.
2. Nothing covers ComposeTerminalDisplay after the rewrite. The DEC 2026 recheck block moved out of a when and into the loop body, and no test asserts that a synchronized update still suppresses redraws and then flushes exactly one on ?2026l. That path has a specific bug history (the "rapid ?2026l/?2026h toggle" comment) and now has no regression net.
3. No test that arrivalNanos stays aligned with dataQueue across the close / sentinel / poll-timeout paths. took() gets all three right today; a test would keep it that way.

Also: FrameLatencyProbeTest mutates the global singleton (enabled plus the shared histograms). Sequential today, so fine - it goes flaky the moment tests run in parallel within one JVM. A withProbeEnabled { } helper or an injectable instance would make it robust.

Security

Nothing of concern. The probe is env-gated, writes only under ~/.bossterm (or an operator-chosen BOSSTERM_FRAME_PROBE_OUT), emits no terminal content into the JSON, catches Throwable in the sampler so a diagnostic cannot take the app down, and uses an atomic rename with a fallback. The shell scripts use set -euo pipefail, quote their paths, and probe.sh handles both BSD and GNU stat.

Performance

Net positive and honestly reported. Removing the debounce means one Main-dispatcher resumption per PTY chunk instead of one per 8/50 ms, but the CONFLATED channel caps pending requests at one and the measured bulk p95 moved the right way (1310 -> 983 ms), so that concern is answered by data rather than by argument. "Draw-call count is not the dominant paint cost" - 86% fewer drawText calls for a 9% paint-cost win on tui - is the most useful result in the baseline file, and steering future renderer work away from layout caching on the strength of it is the right call.

One risk the pixel diff cannot rule out on its own: cellWidth is the averaged advance of W over 100 chars (ProperTerminal.kt:781-790), while a batched run advances by the shaper's own metrics. Merging blanks makes runs much longer, so any advance mismatch now accumulates over a whole line instead of resetting at every word boundary. Clean on the tested font - but the cases that would expose it are a non-monospace fallback font, a fractional cellWidth (odd font size, fractional display scaling), and Windows/Linux DPI, none of which the macOS-only capture covered. Worth one torture-fixture capture per platform before this becomes the default everywhere.

[claude]

Review — perf: measure terminal latency, and cut the per-cell work behind it

Reviewed the full diff and traced each claim back into the source. This is unusually good perf work: the measurement lands before the tuning, the instrument is inert by default, the two invariants that could fail silently (earliest-wins arrival, the ASCII guard) are mutation-checked rather than asserted-by-comment, and the baseline doc corrects its own earlier 99% claim down to a 25% one. "Finding 0" (an occluded macOS window throttles to ~3 fps, 30x larger than anything being measured) is the kind of trap that quietly invalidates a whole benchmark suite, and catching it before publishing numbers is the difference between this and most perf PRs.

Notes ordered by what I'd want resolved before this comes out of draft.

---

1. The performanceMode default change is inert for every existing user

SettingsManager.kt:98-101 sets encodeDefaults = true, explicitly so that all fields are written. So every existing ~/.bossterm/settings.json already contains "performanceMode": "balanced", and flipping the Kotlin default only reaches fresh installs. The benchmark/latency/README.md "what shipped" table and TerminalSettings.kt:634 both present the ~4 ms as delivered; for the installed base it isn't.

Needs a one-time migration (rewrite "balanced" to "latency" on load, guarded by a schema/version bump so a user who deliberately picks balanced later isn't overridden). While in there, PerformanceSettingsSection.kt:187,195 still lists Balanced first and describes it as "Good for most users", which now contradicts both the new default and the KDoc.

2. Two global defaults changed on a single-platform sample

Every figure in LATENCY_BASELINE_2026-08-27.md is macOS / Apple Silicon / one machine. The debounce removal is a bet that Compose's frame clock plus the CONFLATED channel absorb the redraw rate — that's a claim about the frame clock, and Windows (ConPTY, different chunking) and a Linux box without a compositor are exactly where it could differ. HIGH_VOLUME was, whatever else it was, backpressure on Dispatchers.Main.

The harness is the cheap part here — already scripted and platform-neutral. One interactive + one bulk run on Windows and on Linux before this ships as a default would close it. If that isn't practical, say so in the baseline doc rather than letting the table read as cross-platform.

3. Underline colour under batched blanks now follows the run, not the cell

flushBatch (TerminalCanvasRenderer.kt:897-905) draws the underline with batchFgColor across batchText.length cells. Now that blanks extend a run while styleMatches (:1097-1101) deliberately ignores fg for them, an underlined blank inherits the preceding run's colour.

Concretely: ESC[4;31m ab then ESC[4;34m and two spaces — those two blue underlined blanks get a red rule.

This is a net improvement, not a pure regression, and worth saying so: on the old path a blank hit else -> flushBatch() and then if (char != ' ' ...) at :1117, so underlined blanks drew nothing at all. It goes from "missing rule" to "rule in the wrong colour". But the pixel-diff verification can't have caught it — unicode-torture.sh:16 only underlines gaps within a single SGR run, so the colours always agree there. Either add a colour-changing underlined-gap line to the fixture, or track the underline colour separately and flush on a mismatch.

Related nit, pre-existing but now inconsistent: a blank still may not start a run, so ESC[4m + three spaces + text underlines only from t. Interior and trailing underlined blanks now work; leading ones still don't.

4. The probe is a process-global singleton, and there are N tabs and N panes

pendingArrival / pendingTrigger / drawCalls are single slots on the FrameLatencyProbe object, but markArrival fires from every tab's data stream and beginFrame/endFrame fire per pane:

• A background tab's PTY output stamps pendingArrival; the foreground pane's next paint (blink, cursor, selection) consumes it and records a fabricated large byteToPaint, while idleFrames under-counts by the same amount. A build streaming in tab 2 while you read tab 1 poisons the histogram in the pessimistic direction.
• With split panes, each pane's beginFrame resets the shared drawCalls, so drawCallsPerFrame is per-pane, not per-frame, and paintCost gets one sample per pane per frame.

Neither breaks the documented single-tab harness, and neither is worth plumbing a per-pane probe for. But the whole PR rests on these numbers being trustworthy, and benchmark/latency/README.md doesn't say "one tab, no splits, nothing streaming in the background." Given how much care went into the occlusion caveat, this belongs next to it.

Not worth fixing but for the record: enqueue offers to arrivalNanos and dataQueue non-atomically, so with concurrent producers a chunk can be paired with a neighbour's stamp. Distribution-preserving, so the percentiles hold.

5. Gating debug snapshots removes the scrubber's whole point

DebugDataCollector.kt:120 is a good find — 59% of samples in a deep copy nobody reads is exactly what hides behind a default-off feature. I checked the blast radius: MCP read_debug_console (BossTermMcpServer.kt:2192) only touches getDebugChunks(), still recorded unconditionally, so the tool is unaffected. Only DebugPanel reads getSnapshots().

But the panel is a scrubber over history, and the reason to press Ctrl+Shift+D is that something already went wrong. The inline comment answers "does it start showing data immediately?" (yes, next 100 ms tick) rather than the real loss: there is no longer anything to scrub back to. Either keep a much smaller ring at a much lower cadence while disabled (5 snapshots at 1 Hz is ~0.5% of the current cost), or state the tradeoff in the comment — the current one reads as though nothing was given up.

[claude]

(review, part 2 of 2 — smaller findings, what checked out, and test coverage)

Smaller things

• benchmark/latency/README.md:48 documents workloads.sh keystrokes; the script only accepts interactive (workloads.sh:47, usage at :91). Copy-paste from the README fails.
• The PR description is stale. It describes BOSSTERM_FAST_TEXT, BOSSTERM_REDRAW_DEBOUNCE_MS and BOSSTERM_HIGH_VOLUME_DEBOUNCE_MS and an "A/B ladder"; none of the three exist in the final diff, and LATENCY_BASELINE correctly says "no flags remain". Worth rewriting before marking ready — a reviewer arriving cold will go looking for flags that aren't there.
• The ASCII fast path still pays per cell (TerminalCanvasRenderer.kt:975-989): zwjCheckBuilder.get() (a ThreadLocal lookup), setLength(0) and toString() all run even when plainAscii short-circuits the while. Hoisting the whole block into if (!plainAscii) { ... } gets the rest of the win. (The toString() is free — StringLatin1.newString returns "" for length 0 — but the ThreadLocal lookup isn't.)
• RedrawRequest (ComposeTerminalDisplay.kt:50-52) now carries one unused field whose default calls System.currentTimeMillis() on every PTY-driven redraw. Make it an object.
• requestImmediateRedraw vs requestRedraw. The new RedrawRequest KDoc says the two "do exactly the same thing" — and it's right — yet both paths survive, and the immediate one launches a fresh coroutine per call across ~20 call sites. A CONFLATED channel can't drop the last request, so delegating requestImmediateRedraw to requestRedraw is safe and removes the allocation. Optional, but the alternative is a comment saying the distinction is meaningless sitting next to code that maintains it.
• getChunkCount() / getSnapshotCount() (DebugDataCollector.kt:246,253) still call the O(n) ConcurrentLinkedQueue.size() that the rest of that commit exists to avoid. The counters are right there.
• Stale docs: requestRedraw's KDoc still says "applies debouncing" (:356) and the trySend comment still calls conflation "the intended debouncing behaviour" (:368).
• TerminalCanvasRendererTestAccess is dead weight — its comment says file-private top-level functions aren't reachable from a test, but isPlainAsciiRun is declared internal, and the same test file already calls visibleRunLength directly.
• FrameLatencyProbeTest:47 says "Four sub-buckets per octave"; SUB_BITS = 3 means eight, which is what the production KDoc says. The 1/8 bound in the assertion is correct either way.
• AGENTS.md wasn't touched. "The adaptive debounce was removed because it cost 14.6 ms of echo latency to buy redraw-count reduction the frame clock already provided" is exactly the class of measured, do-not-re-add fact that file exists to hold — as is finding 0.

Things I checked that hold up

• isPlainAsciiRun's col..col+2 window really does cover all three probes: checkRegionalIndicatorSequence requires a high surrogate at col; checkFollowingSkinTone reaches at most col+2 across a DWC marker; and the ZWJ branch only fires when graphemes[0].hasZWJ, which for an ASCII cell needs the ZWJ at col+1 (the a<ZWJ>b corpus entry catches this). CharUtils.DWC is U+E000, non-ASCII, so DWC neighbourhoods correctly decline the fast path. charAt past the line end returns EMPTY_CHAR (a space), so the i >= bufferLimit early return can't mask a non-ASCII cell.
• Backgrounds are pass 1 and per cell (:804-821), so blanks joining a text batch can't affect bg, inverse or dim fills.
• Cells past line.length() read as unstyled blanks, so isUnderline is false there and a line-ending underline stops correctly rather than running to the right edge.
• Every dataQueue take is wrapped in took(), and CLOSE_SENTINEL is offered raw at :227 and skipped inside took, so the two queues stay aligned across close.
• The histogram bucketing round-trips (checked indexOf/valueOf at 8, 15, 16, 17 and the 255 clamp), and reporting the bucket floor is the right direction for a latency claim — it cannot manufacture an improvement that isn't there.

Test coverage

The three new suites cover the right things, and extracting visibleRunLength precisely because it's where a character could get dropped is the right instinct. Gaps:

• The batching decision itself is untested. visibleRunLength got extracted and pinned; canBatch / styleMatches (:1092-1101) did not, and that's where the risk actually moved — a blank must not start a run, an underlined blank must not join a non-underlined one, NUL must append as a space rather than a control char, a blink-off blank must flush. The same extraction treatment makes all four cheap.
• Nothing pins the arrival-queue alignment. A test that a CLOSE_SENTINEL round-trip leaves arrivalNanos empty, and that nothing is enqueued at all when the probe is off, would catch the failure mode that makes every later measurement wrong-but-plausible.
• FrameLatencyProbeTest mutates process-global state (enabled, reset()) on a singleton. Fine today; it will bite the first time the suite forks in parallel alongside anything that paints.

No security concerns — the probe is opt-in via env var, writes only timing aggregates, and the reset marker is a local file with no privileged effect.

[claude]

Review: perf/terminal-latency (part 1/2)

Read the whole diff plus the surrounding code in TerminalCanvasRenderer, ComposeTerminalDisplay, BlockingTerminalDataStream, DebugDataCollector, SettingsManager and TerminalEmulatorDrain. I could not run Gradle in this environment, so everything below is from reading the code — no build or test verification from me.

First, plainly: the methodology in benchmark_results/LATENCY_BASELINE_2026-08-27.md is the best thing in this PR. Finding 0 (occluded-window throttling swamping the signal), the retracted 99% claim, and "a fix that did not work, and why it looked like it would" — a whole-JVM profile answering "where is CPU spent" rather than "what is the critical path" — are exactly the failure modes that make perf work untrustworthy, and they are documented rather than buried. The three bulk-output fixes (per-character IllegalThreadStateException, AWT event per redraw request, ICU on ASCII) are all real and all invisible from reading the code, which is the point.

The findings below are mostly about the rendering change, which is the one part of this PR that fails silently.

---

1. Blank batching changes underline rendering, and inconsistently

canBatch previously excluded ' ' and NUL, so an underlined blank hit the else branch, flushed the batch, and then drew nothing — underlined spaces never got a rule. Now a blank extends a run whose batchIsUnderline matches, and flushBatch draws drawLine across batchText.length. So (ESC written out):

ESC[4mfoo   barESC[0m       # BEFORE: two short rules with a gap.  NOW: one continuous rule.
ESC[4mtrailing    ESC[0m|   # BEFORE: rule stops at "trailing".    NOW: rule reaches the "|".

Both are the correct xterm behaviour, so I read this as a fix. But it is a visible change, and it is asymmetric:

val canBatch = … && (!isBlankCell || batchText.isNotEmpty())

A blank may not start a run, so ESC[4m fooESC[0m still draws no rule under the four leading spaces while ESC[4mfoo ESC[0m now draws one under the four trailing ones. Before this PR the answer was uniformly "no rule on blanks"; now it depends on whether an underlined non-blank happens to precede the blank on that row. That is harder to reason about than either extreme.

This also means benchmark/latency/unicode-torture.sh's underline row cannot have been pixel-identical, which the baseline doc claims for the whole fixture ("pixel-identical apart from subpixel antialiasing … underlines spanning gaps"). Worth re-checking that comparison specifically — if the underline row really came out identical, the diff was not seeing what you think it was.

Relatedly, batchFgColor is captured from the run's first cell and styleMatches now ignores fg for blanks, so ESC[4;31mredESC[4;32m ESC[0m draws the spaces' rule in red. Pre-existing for non-blanks; new for blanks.

2. visibleRunLength(text, underlined = true) is dead weight

flushBatch draws the underline with batchText.length, not drawLength:

val drawLength = visibleRunLength(batchText, batchIsUnderline)
if (drawLength > 0) { drawTextClipped(text = batchText.substring(0, drawLength), …) }
if (batchIsUnderline) {
    val underlineWidth = batchText.length * ctx.cellWidth   // <- independent of drawLength

So the underline never needed the untrimmed length. Returning text.length for underlined runs just hands the shaper trailing invisible spaces to lay out — the exact cost the function exists to avoid, on precisely the rows (aligned tables under a heading rule) where it is largest. The underlined parameter can go, along with two of the four VisibleRunLengthTest cases; the "the blanks carry the rule" comment is describing drawLine's behaviour, not drawText's.

3. The performanceMode default flip will not reach a single existing user

SettingsManager uses encodeDefaults = true ("Ensure all fields are written, not just non-default ones", SettingsManager.kt:101) and re-saves on any round-trip mismatch (:297). So every existing ~/.bossterm/settings.json already contains "performanceMode": "balanced" verbatim, and changing the Kotlin default is invisible to those users. The measured ~4 ms (16.4 -> 12.3) that this line is credited with lands only on fresh installs.

The repo already has the pattern for this — migrateCursorRenderingDefaults and migrateSplitFocusBorderDefault, gated on a "…Version" in rawSettings sentinel, two lines above the load call. A changed default needs one of those or it is not a change.

4. The probe is process-wide, but the thing it measures is per-pane

pendingArrival, pendingTrigger and drawCalls are singletons on object FrameLatencyProbe, while renderTerminal runs once per ProperTerminal and markArrival fires for every BlockingTerminalDataStream in the process. Two consequences:

• Output arriving in a background tab is stamped into pendingArrival but that tab never paints. The next foreground paint does pendingArrival.getAndSet(NONE) and records the whole interval as its own byteToPaint — an arbitrarily large fabricated sample, in the direction that makes a build look worse.
• In a split pane, whichever pane paints first consumes the arrival and resets drawCalls, so drawCallsPerFrame is per-pane-that-won-the-race.

Given that this PR's defaults are justified by these numbers, I would either scope the probe per-display or state "one tab, one pane" as a precondition in benchmark/latency/README.md. The finding-0 note already sets the precedent for that kind of caveat, and this one is the same shape: an artefact much larger than the effect.

5. RedrawRequest still costs what redrawQueued was added to remove

The pending-flag fix is sound — I traced the clear-before-redraw ordering and the isFailure release, and I do not see a lost wakeup or a latch. But:

data class RedrawRequest(val timestamp: Long = System.currentTimeMillis(),)

Nothing reads request in the processor loop any more. So every surviving redraw request still allocates an object and reads the wall clock on the emulator thread, per buffer mutation — the same hot path whose AWTEvent.<init> cost you just measured at 20%. Channel<Unit> + trySend(Unit) drops both. (The trailing comma is legal but reads like a leftover.)

[claude]

Review: perf/terminal-latency (part 2/2)

6. The ASCII fast path still pays for the lookahead it skips

val builder = zwjCheckBuilder.get()
builder.setLength(0)
run { while (!plainAscii && …) { … } }
val cleanText = builder.toString()

On the fast path this is still a ThreadLocal lookup, a setLength(0), and a StringBuilder.toString() on an empty builder — per cell, per frame, which is the granularity the change is about. Hoisting the whole block behind if (!plainAscii) (and giving cleanText an "" else-branch) gets the rest of the win and reads better than a loop-invariant condition inside the while.

The guard itself I believe is correct, and I checked it rather than taking the comment's word:

• regional indicators require a high surrogate at col, so col alone rules them out;
• checkFollowingSkinTone only reaches col+3 when charAt(col) is a high surrogate, i.e. already non-ASCII;
• for ZWJ, UAX#29 GB9 makes a<ZWJ>b a single cluster, so col+1 is the furthest a ZWJ can be pulled into graphemes[0]; a ZWJ at col+2 or beyond cannot, because 'a'/'b' break at GB999. col..col+2 therefore has a cell of margin.
• CharUtils.DWC is U+E000 (>= 0x80), so any DWC marker inside the window makes the guard decline. Conservative, correct.

7. Reuse: GraphemeCluster.ASCII_CLUSTERS already exists

GraphemeUtils.asciiClusters + asciiCluster() re-implements GraphemeCluster.Companion.ASCII_CLUSTERS, which interns all 128 ASCII clusters and is reachable as GraphemeCluster.fromChar(ch, 1) (returns the cached instance when the width matches, which it always does for 0x20..0x7E). Using fromChar removes the duplicate and the Array(0x7F - 0x20) size expression — correct at 95, but it reads like an off-by-one; 0x7E - 0x20 + 1 says what is meant.

I checked that nobody mutates GraphemeCluster.codePoints before flagging this; interning is safe either way.

8. Stale KDoc on segmentIntoGraphemes

GraphemeUtils.kt:157-183 now has two consecutive KDoc blocks: the original "Segments a string into grapheme clusters … @PARAM text @return List of grapheme clusters" is orphaned above isAllPrintableAscii, and segmentIntoGraphemes at :193 has no doc at all. IDE hover and Dokka will both attribute the wrong one.

9. isAlive() caching widens an existing busy-spin window

Good fix, and the 20 ms TTL plus death-latching is the right shape. One knock-on: two PTY reader loops do not break on a null read():

• TabController.kt:1764 — while (handle.isAlive()) { val output = handle.read(); if (output != null) … }
• EmbeddableTerminal.kt:1102 — same shape

TerminalSessionCore.kt:236 has an explicit comment about exactly this hazard and does ?: break ("continue here would busy-spin at 100% CPU in the window before isAlive() flips false"). The cache extends that window by up to 20 ms. Bounded and minor, but it is a one-line ?: break in each to match the daemon, and this PR is the reason to do it.

10. Smaller things

• DebugDataCollector.trim counter drifts low under concurrency. recordChunk is called from the PTY reader, the UI thread (USER_INPUT) and the console-log path, so two threads can both observe count.get() > max before either decrements, and both poll — the ring under-fills by one. Self-healing and harmless, but DebugDataCollectorTrimTest is single-threaded, so it pins the invariant the drift does not threaten. Worth a sentence in the KDoc that the bound is approximate under concurrent recording.
• Gating captureState on debugEnabled is a good catch, and I confirmed the only snapshot consumer is DebugPanel (MCP read_debug_console reads chunks, not snapshots), so nothing loses data it was using. Note the panel now opens on an empty snapshot list for one debugCaptureInterval instead of showing the preceding ~10 s of history.
• markArrival's KDoc is wrong about where it is called. It says "Called from the data stream the moment a chunk is handed over, ahead of the queue wait" — it is actually called from took() at dequeue time, with the append-time stamp. The measurement is right; the doc points at the wrong line.
• arrivalNanos alignment is not guaranteed. enqueue offers the stamp and the chunk as two separate operations, so two concurrent append() calls can pair chunk B with stamp A; and chunks still queued at close() leave their stamps behind. Diagnostic-only, but the KDoc claims the two queues stay aligned — worth softening to best-effort.
• FrameLatencyProbe.report() is dead code (no call sites) and already out of sync with json(): it omits triggerToPaint and queueWait, the two series the baseline doc leans on hardest.
• FrameLatencyProbeTest mutates process-wide state. It flips FrameLatencyProbe.enabled and resets the shared histograms in @BeforeTest. Any other test in the same JVM that appends to a BlockingTerminalDataStream or paints while this class runs will pollute it or be polluted by it. Most assertions already use a locally-constructed Histogram(); the three that use the singletons (frameKeepsTheEarliestPendingArrival, aFrameWithNoNewDataIsNotCountedAsLatency, drawCallsAreCountedPerFrameNotCumulatively) are the exposure. Its comment also says "Four sub-buckets per octave puts the worst case at 1/8" — it is eight (SUB_BITS = 3); the class KDoc has it right.
• PlainAsciiFastPathTest's ZWJ leg asserts the guard's own predicate (charAt(col).code < 0x80) rather than comparing against segmentIntoGraphemes(lookahead)[0].hasZWJ, the way the other two legs compare against the real probes. It does catch a narrowed guard, so it is not vacuous, but it is the weakest of the three and covers the subtlest rule. Also the corpus is built straight from Strings, so no line contains a CharUtils.DWC marker — the layouts real buffers hold for wide chars are not exercised (safe by accident, per 📝 Add context menu / right-click menu #6).
• TerminalCanvasRendererTestAccess is unnecessary. Its comment says "File-private top-level functions are not reachable from a test" — but isPlainAsciiRun is internal, and the same file calls the equally-internal visibleRunLength directly two classes down.
• benchmark/latency/README.md:48 documents workloads.sh keystrokes; the script only accepts interactive|bulk|tui|scroll|aged.
• The PR description is stale. It describes three env vars (BOSSTERM_REDRAW_DEBOUNCE_MS, BOSSTERM_HIGH_VOLUME_DEBOUNCE_MS, BOSSTERM_FAST_TEXT) and says "With none set the build behaves exactly as shipped, so a run with no env is a true baseline" — none exist at head, and the baseline doc correctly says "no flags remain". Fine for a draft, but the body is now the most misleading document in the PR.

---

Parts I checked and think are right

• The redrawQueued CAS / clear-before-redraw ordering, including the sync-update continue path, the isFailure release, and the processor restart loop. No lost wakeup that I can find.
• The Histogram bucketing: indexOf/valueOf round-trip contiguously across the sub-linear boundary at 8, and quantile reporting the bucket floor biases every number downward — the safe direction for the claim being made.
• Dropping IMMEDIATE/NORMAL priority: with the debounce gone the two paths differ only in whether they go through the channel, and requestImmediateRedraw still exists for the ~16 input call sites, so scrollArea falling back to requestRedraw() is the right simplification.
• A side effect of the performanceMode flip worth knowing, and arguably a second win: getChar's BALANCED branch polls with a 10 ms timeout, so an idle tab was invoking onTerminalStateChanged ~100x/sec forever. LATENCY uses a blocking take(), so that becomes once per data arrival. Type-ahead validation still fires on every arrival, which is what it needs, and an idle terminal stops waking up.

Summary

The measurement infrastructure and the three bulk-output fixes are strong; I would take them as-is modulo the doc and dead-code nits. The two I would want resolved before this stops being a draft are #3 — the settings default does not actually ship, so one of the two headline wins is currently a no-op for existing users — and #1/#2, since the underline behaviour change is real, is inconsistent depending on what precedes the blanks, and contradicts the pixel-comparison claim that is the only verification the renderer change has.

[claude]

Review (1/2): correctness + the measurement table

Unusually good performance PR. The measurement-before-tuning discipline, the mutation-checked invariants, the self-correction of the "~99% collapse" claim, and the "a fix that did not work, and why it looked like it would" section are all things I'd like to see more of. Most of what follows is either (a) the results table not having kept up with the last three commits, or (b) leftovers on paths the PR itself just optimised.

Reviewed by reading; I did not build or run anything (per AGENTS.md).

Correctness arguments I checked and agree with

Stating these explicitly, because they're the three places a silent wrong-picture bug would live:

• visualColToBufferCol fast path is exactly equivalent. Every skip condition in shouldSkipChar is non-ASCII (DWC U+E000, variation selectors, ZWJ, surrogates, gender symbols), so on a line with the flag clear nothing is skipped, charWidth is always 1, and the early-return visualCol < currentVisualCol + charWidth can never fire inside the loop — the slow path returns col == min(visualCol, width). And myRequiresVisualColumnMapping is maintained sticky-true (TerminalLine.kt:359, :382 OR into it; :167/:225/:334 are whole-line recomputes), so the guard can only be conservatively true, never falsely false. That's the safe direction and it's the load-bearing fact — worth one line in the ColumnConversionUtils comment, since a refactor that made the flag non-sticky would break this silently.
• isPlainAsciiRun never skips a probe that would have fired. hasZWJ only matters via graphemes[0].hasZWJ, and a cluster starting on an ASCII code unit can only pull a ZWJ in from col + 1; checkFollowingSkinTone reaches col + 2 at most (DWC step); checkRegionalIndicatorSequence needs a high surrogate at col itself. col..col+2 covers all three.
• The redrawQueued protocol is sound. "Flag true ⇒ at least one item queued" holds because the processor clears the flag before actualRedraw(), and a mutation that loses the CAS is necessarily followed by a redraw reading post-mutation state. The isFailure → set(false) release correctly avoids latching on a closed channel.
• performanceMode = "latency" introduces no spin. getChar uses blocking take() in LATENCY (cheaper than BALANCED's 10 ms poll) and readNonControlCharacters breaks on a null poll. So the new default is both lower-latency and lower-CPU on the blocking path. Good change.

1. The results table mixes commits, and two rows regressed without being called out

LATENCY_BASELINE_2026-08-27.md presents one "shipped defaults" column, but the bulk/tui/scroll byteToPaint rows were taken at e8e04af, before 610538b, 20898ba and 07e12cd landed. Only queueWaitMs was re-measured after those. The PR is candid about this for the debug-collector commit ("NOT YET VERIFIED BY MEASUREMENT") but the final table doesn't carry the caveat forward.

As written the table says:

workload	metric	baseline	shipped
bulk	p50	81.9	491.5	6x worse
bulk	paintCost p50	1.41	4.10	3x worse
bulk	drawCalls p50	20	128	6x more
scroll	p95	73.7	98.3	33% worse
tui	p50	9.2	11.3	23% worse

The prose only discusses the wins. I don't think the bulk p50 is real — almost certainly a regime artefact (far fewer, much fatter frames, which also explains draw calls going up on bulk while dropping 86% on tui). But that's precisely the argument for re-running rather than leaving it under a heading that says "shipped defaults". The scroll p95 regression looks more likely to be genuine — see #2.

Suggestion: re-run the full {interactive,bulk,tui,scroll,aged} matrix on 07e12cd and replace the table, or split it into per-commit columns. Given how carefully the rest of the file separates measured from inferred, this is the one place a reader could be misled.

2. scrollArea changed dispatch path — plausible cause of the scroll p95 regression

scrollArea went from requestImmediateRedraw() (direct actualRedraw() on Main) to requestRedraw() (CONFLATED channel + redrawQueued gate) for the interactive case. That's the one path the scroll workload hammers, and scroll p95 is the row that got worse. Worth A/B-ing that single line before merge — if it's the cause, scrollArea can keep the direct path at no cost now that there's no debounce to bypass.

3. drawCallsPerFrame is contaminated by the cursor overlay

renderCursorOverlay calls drawTextClipped (TerminalCanvasRenderer.kt:1602), which now calls FrameLatencyProbe.countDrawCall(). But the cursor lives in its own Canvas, outside renderTerminal's beginFrame/endFrame pair:

• on a normal frame the cursor's draw call lands after endFrame() and is charged to the next frame (+1 systematic bias);
• on a blink-only repaint — exactly the case the separate overlay Canvas exists to make cheap — renderTerminal never runs, so increments accumulate with no beginFrame() reset until the next real paint.

208 → 30 and 176 → 60 are off by roughly one, so "draw-call count is not the dominant paint cost" survives. But it's a measurement defect in the file whose whole thesis is measurement hygiene. Fix: a countDrawCall: Boolean = true param on drawTextClipped, passed false from the cursor overlay; or make countDrawCall() a no-op unless a frame is open.

[claude]

Review (2/3): leftovers on paths this PR just optimised, and stale docs

4. requestImmediateRedraw is now the slower path, and two doc comments are false

The class doc says "an 'immediate' and a 'normal' request do exactly the same thing", and requestImmediateRedraw's own KDoc still says "bypasses debouncing" / "guarantee zero lag". Neither holds after this PR:

• there's no debouncing left to bypass;
• requestImmediateRedraw launches a fresh coroutine on Main per call with no coalescing, while requestRedraw collapses N calls into one dispatch via redrawQueued. So immediate is now strictly more expensive, across ~20 call sites including mouse-wheel and selection-drag paths that fire in bursts.

Also worth correcting: the class comment credits the frame clock for bounding over-rendering. The frame clock bounds paints; what bounds Main-thread dispatches — which the 8 ms sleep capped at 125/sec — is now redrawQueued. That's the actual safety argument for removing the sleep, and it deserves to be the one written down.

Either collapse the two entry points (the "initial prompt didn't display" bug the CRITICAL FIX comment describes can't recur under the flag protocol — conflation cannot lose state, because the processor clears the flag before reading), or keep both and fix the comments.

5. RedrawRequest.timestamp is dead and costs a clock read per redraw request

data class RedrawRequest(val timestamp: Long = System.currentTimeMillis()) — nothing reads timestamp any more. That's a System.currentTimeMillis() on every buffer mutation, on the exact path 20898ba just spent effort de-costing. Make it Channel<Unit> and drop the class.

6. The ASCII fast path still pays the ThreadLocal lookup and the toString()

cleanText is only read inside if (hasZWJ || hasSkinTone || hasRegionalIndicator), but zwjCheckBuilder.get() and builder.toString() still run unconditionally, per cell, per frame — the while (!plainAscii && ...) guard only elides the loop body. So "the 20-char lookahead, the String it builds and the three scans are dead work" is two-thirds delivered. Moving all three inside if (!plainAscii) { ... } is a free win and makes the claim true.

7. Docs and the settings UI still say the default is balanced

Not reflected anywhere outside TerminalSettings.kt:

• README.md:345 — example settings block shows "performanceMode": "balanced"
• docs/wiki/Configuration.md:33 and :91
• docs/wiki/API-Reference.md:303 — val performanceMode: String = "balanced"
• docs/wiki/Frequently-Asked-Questions.md:242 — | balanced | Default, auto-adjusts | General use |
• settings/sections/PerformanceSettingsSection.kt:185 — dropdown lists Balanced first, and its description ("Good for most users") reads as the default

8. Existing users won't get the change

performanceMode is persisted in ~/.bossterm/settings.json, so any existing install keeps "balanced" and none of the measured 4 ms reaches it. Either migrate on load (treat a stored "balanced" as "latency" once, or bump a settings version) or state in the PR that the win is new-installs-only. Worth deciding deliberately rather than by omission, since the whole point of that commit is the default.

9. benchmark/latency/README.md documents a subcommand that doesn't exist

README.md:48 says workloads.sh keystrokes; the script only accepts interactive|bulk|tui|scroll|aged. Anyone following the README fails at step 3 of 4.

Related: the README calls it "200 single keypresses at a prompt". It isn't — the script printfs to its own stdout, so it's terminal output, not input. workloads.sh's own header is accurate ("measures the output half of the loop"); the README should match, since the input half is exactly what the camera anchor exists for.

10. The tui workload can strand the user on the alternate screen

\033[?1049h ... \033[?1049l with no trap. Ctrl+C mid-run (likely — it's a 10 s loop) leaves the alt screen active.

trap 'printf "\033[?1049l"' EXIT INT TERM

11. The debug panel loses retroactive history

Gating captureState() on currentTab.debugEnabled.value is clearly right — 10 full-buffer deep copies per second per tab for a panel that defaults off is indefensible, and getSnapshots() has exactly one consumer (DebugPanel.kt:51), so nothing breaks. But behaviour does change: previously opening the panel gave you up to maxSnapshots of what just happened; now it starts empty and only fills forward. For a "something weird just occurred, let me look" tool that's a real loss. If it's an accepted trade, say so in the doc comment (which currently only explains why the loop keeps ticking); if not, a much cheaper always-on capture (cursor + dimensions + screen-only shallow snapshot, no history walk) preserves it at a fraction of the cost.

[claude]

Review (3/3): test coverage, nits, and what to keep

12. Test coverage — the riskiest new pieces have none

Extracting visibleRunLength specifically so it could be tested is the right instinct; it just wasn't applied to the others.

• redrawQueued is the highest-consequence new code here: if the flag ever latches true, the terminal stops painting forever, with no exception and no log — the failure mode is "app appears frozen". Currently protected by a comment. The claim/release logic is pure and could be lifted out of the Dispatchers.Main coroutine behind a seam (trySendFn: () -> Boolean), then tested for claim -> send -> clear -> reclaim, send-failure releases, and mutation-during-redraw still queues.
• The batch-join predicate. canBatch / styleMatches is where a wrong picture comes from, and it's pure boolean logic over (isBlank, batchEmpty, fg, bold, italic, underline). Extract canJoinRun(...) the way visibleRunLength was and pin four things: a blank may not start a run; a blank joins across an fg/bold/italic mismatch; a blank does not join across an underline mismatch; a non-blank does not join across an fg mismatch. That complements the pixel diff rather than duplicating it.
• isAlive() — TTL and death-latching, against a fake ProcessHandle. Note the 20 ms staleness is also observable via TerminalSessionCore.isAlive() -> SessionHost.kt:221 -> daemon list_sessions, so a session can report alive: true for up to 20 ms after exit. Almost certainly fine, but it's now a public-API property rather than an internal loop detail.
• arrivalNanos / dataQueue alignment — see below.

13. Smaller things

• arrivalNanos and dataQueue are two queues kept in step non-atomically. enqueue offers the stamp then the chunk, so two concurrent appends can interleave and pair a chunk with the wrong stamp; chunks left in dataQueue at close leak their stamps; and FrameLatencyProbe.reset() doesn't clear arrivalNanos, so a probe.sh reset mid-stream can leave the two permanently offset. Probe-only, nothing user-visible — but it is the instrument, and the PR's own standard is that the instrument gets checked rather than trusted. One ConcurrentLinkedQueue<Stamped> makes it exact for the cost of one allocation per chunk when probing, which is already the case.
• FrameLatencyProbeTest.bucketsRoundTripWithinStatedError comment says "Four sub-buckets per octave puts the worst case at 1/8". SUB_BITS = 3 -> eight. The Histogram KDoc has it right (and explains why four would be 1/4); the test comment inverts it.
• TerminalCanvasRendererTestAccess — its comment says "File-private top-level functions are not reachable from a test; this is." isPlainAsciiRun is internal, and VisibleRunLengthTest in the same file calls visibleRunLength directly. The wrapper and the comment can both go.
• aliveCheckedAtNanos == 0L as the "never checked" sentinel — System.nanoTime() has an arbitrary origin and may legitimately return 0, in which case the cache never engages. Harmless (extra syscalls only), but a separate everChecked flag says what's meant.
• asciiClusters interning shares one IntArray per character process-wide. GraphemeCluster.equals is structural (GraphemeCluster.kt:88), so no semantic change — I checked. But a // shared instance - never mutate codePoints note would keep the next person out of trouble.
• DebugDataCollector.clear() racing a concurrent recordChunk (offer landed, incrementAndGet not yet) leaves the counter one short permanently, so the ring settles at max + 1. Benign and self-limiting; noting only because the new test file's premise is that counter drift is worth pinning.
• workloads.sh writes fixtures to a predictable ${TMPDIR:-/tmp}/bossterm-latency. Fine for a dev script on a single-user box; flagging in case these ever run in shared CI.

Things I'd keep exactly as they are

• The earliest-arrival CAS rule, and that a wrong choice there is caught by a named test (frameKeepsTheEarliestPendingArrival) rather than a comment.
• Bucket floors for quantiles, with the reason spelled out: the instrument cannot manufacture an improvement.
• idleFrames counted separately instead of recorded as near-zero latency — the difference between a probe and a marketing number.
• Finding 0 (occlusion -> ~3 fps) documented before the results, with the earlier bad numbers named as the artefact they were.
• Splitting segmentViaBreakIterator out so the fast path is provable against the incumbent rather than argued, plus theFastPathActuallyEngages guarding against a guard that never fires. Same for ColumnConversionFastPathTest's note that the first version of that test passed against a build with the guard deleted — the single most useful sentence in the diff.
• TerminalLineWriteModelTest landing ahead of the merge rewrite, with the reason the rewrite isn't here (run coalescing, charAt becoming O(entries), degradation only visible in a long session). Correct call.

The one blocker for me is #1 — re-run the matrix on HEAD so the table describes the build it's headed. #2, #3, #5 and #6 are small and worth folding in while you're there.

[claude]

Review: measure terminal latency, and cut the per-cell work behind it — part 1/2

Read the whole diff plus surrounding code in TerminalCanvasRenderer, ComposeTerminalDisplay, TerminalLine, ColumnConversionUtils and GraphemeCluster. I did not build or run anything (per AGENTS.md, and gradle was not reachable here) — everything below is static analysis.

This is unusually good work. Measuring before tuning, splitting byteToPaint at the redraw trigger so "slow recompose" and "long debounce" are distinguishable, publishing the correction when the 99% bulk claim turned out to be a 33-sample artefact, and keeping the DebugDataCollector fix while explicitly saying it bought nothing on the critical path — that is the right shape for a perf PR. The mutation-checked invariants are the part I would most want other PRs to copy.

I verified three load-bearing arguments independently and they hold:

• The col..col+2 guard width is exactly right, not approximately. checkRegionalIndicatorSequence needs only col; the ZWJ branch only ever consults graphemes[0], so ASCII at col/col+1 rules it out (a ZWJ at col+7 set hasZWJ before this PR too, but grapheme.hasZWJ was false and it fell through — no behaviour change); checkFollowingSkinTone can reach col+2 after stepping a DWC. Narrower would be wrong, wider would be waste.
• The visualColToBufferCol fast path is exactly the slow path. For an all-width-1 line the early return at :145 can never fire (visualCol < currentVisualCol + 1 contradicts the loop guard), so the slow path also returns min(visualCol, width). And myRequiresVisualColumnMapping is sticky-true, which is the safe direction.
• redrawQueued has no lost wakeup. The window I went looking for — producer CAS fails between the processor receive() and its set(false) — is safe: the mutation necessarily precedes the CAS read, which precedes set(false), which precedes actualRedraw() and therefore the capture. Worth stating in the comment, since it is the non-obvious part.

---

Correctness

1. GraphemeUtils.asciiClusters duplicates a cache that already exists — GraphemeUtils.kt:186-191

GraphemeCluster.Companion.ASCII_CLUSTERS (GraphemeCluster.kt:114-121) is already a 128-entry interned array with visualWidth = 1 for exactly 0x20..0x7E, and fromChar(c, 1) already returns the cached instance for that range. The new 95-entry array and asciiCluster() can just be:

for (ch in text) add(GraphemeCluster.fromChar(ch, 1))

Same interning, no second table, no way for the two to drift if the width rule for some ASCII range ever changes.

2. isPlainAsciiRun pays 3 x O(runs) per cell — TerminalCanvasRenderer.kt:419-425, called at :972

TerminalLine.charAt is a linear walk over myTextEntries (TerminalLine.kt:199-212), so the guard costs three run-walks per cell, per frame — on precisely the lines the PR targets. A ls --color row, a diff or a powerline prompt has tens of entries, so that is real added work on top of the ~20 charAt calls it saves, and the non-ASCII path now pays it and then does the old work anyway.

TerminalLine already maintains the predicate you want, and this PR sibling change relies on it: !line.requiresVisualColumnMapping implies no char above 0x7F anywhere on the line, which implies isPlainAsciiRun is true at every column. Hoisting one field read out of the column loop replaces 3 x cols x runs walks with one. It needs a type-ahead guard (see item 3), with the neighbourhood probe as fallback:

val lineIsAscii = line.myTypeAheadLine == null && !line.requiresVisualColumnMapping
// in the loop:
val plainAscii = lineIsAscii || isPlainAsciiRun(line, col, bufferLimit)

3. Does the visualColToBufferCol fast path see the type-ahead shadow line? — ColumnConversionUtils.kt:129

charAt delegates to myTypeAheadLine when one is set (TerminalLine.kt:200-203), but requiresVisualColumnMapping returns the base line flag — the type-ahead line is a copy() written through its own writeString, so it maintains its own. If a prediction ever contains a char above 0x7F (an IME commit, or a paste-echo prediction), the base flag stays false while charAt reports a wide char, and the fast path returns the identity where the slow path would not.

Narrow: type-ahead is transient and the emulator-side wrapLines caller is unaffected (TerminalTextBuffer.kt:1073 clears predictions on output), so the blast radius is mouse hit-testing/selection being a cell off for tens of ms. But the framing here is "the guard IS the correctness argument", so worth either confirming predictions are ASCII-only by construction or adding myTypeAheadLine == null to the guard.

4. Underlined blanks are now asymmetric — TerminalCanvasRenderer.kt:1092-1101, 403-408

A blank may extend a run but not start one, so ESC[4m followed by three spaces then text still draws no rule under the leading spaces (they hit canBatch == false, then flushBatch(), then the char != space skip), while trailing underlined spaces now do get one. Before this PR neither did, so it is a partial fix rather than a regression — but the two ends of the same underline now behave differently.

Related: flushBatch draws the rule with batchFgColor, and blanks bypass the colour check, so an underlined gap whose SGR changed colour mid-run is drawn in the run starting colour.

unicode-torture.sh covers underlined with gaps and trailing — worth adding a leading-blank case and a colour-change-across-the-gap case to the same fixture, since those are the two it cannot currently see.

5. isAlive() cache is not invalidated by kill() — PlatformServices.desktop.kt:297-306

Death is latched, but there is no aliveCheckedAtNanos = 0 in kill(), so a caller polling right after a kill sees a stale true for up to 20 ms. I could not find a caller where that matters (the sites in TabController/TerminalSessionCore are while (handle.isAlive()) watch loops that just exit 20 ms late), so this is a while-you-are-here rather than a defect. One line in kill() makes it exact.

[claude]

Review — part 2/2: performance, harness, docs, tests

Performance

6. RedrawRequest allocates and reads the clock on every redraw request, and nothing reads the result — ComposeTerminalDisplay.kt:299-301, 338, 391

The processor loop destructures nothing from request. Given this PR own finding that requestRedraw was 20% of the parse thread under load, Channel<Unit> / trySend(Unit) drops an allocation and a System.currentTimeMillis() from the same hot path you just optimised. Small, since redrawQueued already means it fires far less often — but free.

7. The redraw processor no longer yields — ComposeTerminalDisplay.kt:337-358

With delay(mode.debounceMs) gone, the loop on Dispatchers.Main drains as fast as the producer refills: receive, set(false), actualRedraw, receive. redrawQueued bounds it to one outstanding item, and your measurement (frames stayed vsync-capped at ~62/s) says it is fine in practice — so this is a question, not a finding: under sustained bulk output, is Main ever spending measurable time on trigger bumps between frames?

If you ever want the belt-and-braces version, withFrameNanos is the direct expression of the model the PR cites from the Rust implementation — coalesce against the frame clock rather than a channel — and it self-limits without reintroducing a sleep.

8. Probe state is process-global, but tabs are not — FrameLatencyProbe.kt:75-81

pendingArrival, pendingTrigger and drawCalls are singleton state, while every tab has its own BlockingTerminalDataStream and only the active one paints. A background tab producing output stamps pendingArrival and the foreground tab next paint consumes it, so byteToPaintMs silently becomes garbage in any multi-tab session and idleFrames under-counts. Same class of trap as Finding 0 in the baseline doc, and worth the same treatment: a line in benchmark/latency/README.md saying one tab, no background output.

---

Harness bugs

9. probe.sh reset can report a false success on Linux — benchmark/latency/probe.sh:137,144

stat -f %m "$OUT" 2>/dev/null || stat -c %Y "$OUT" has the platforms backwards. Verified against GNU coreutils: stat -f %m file treats %m as a second operand, writes the filesystem block for $OUT to stdout, and exits 1 — so the fallback also fires and before/now both become a multi-line blob (Blocks: Total/Free/Available, then the mtime). The mtime comparison then works by accident, but Free:/Available: drift on their own with any unrelated disk write, so "$now" != "$before" can be true when the sampler never rewrote the file. That is a false "reset confirmed", i.e. a workload measured against the previous run counters. Try GNU first; it fails cleanly on BSD:

stat -c %Y "$OUT" 2>/dev/null || stat -f %m "$OUT"

10. README documents a workload name the script rejects — benchmark/latency/README.md:54 says workloads.sh keystrokes; the script case arm and its usage string are both interactive.

---

Docs and consistency

11. Three stale "debouncing" references in the file that removes debouncing — ComposeTerminalDisplay.kt:318 (the ADAPTIVE DEBOUNCING LOGIC banner), :321 ("handles debouncing"), :375 ("normal priority, applies debouncing"). requestImmediateRedraw "bypasses debouncing" at :462 likewise describes something that no longer exists. Given how carefully the rest of the comments were rewritten, these read as oversights.

12. The new performanceMode KDoc numbers describe only one of two poll sites — TerminalSettings.kt:586-588 says throughput 10 ms / balanced 5 ms, which matches BlockingTerminalDataStream.kt:321-325 but not :262-267, where the same three modes are take() / 100 ms / 10 ms. Worth naming both sites, or the measured-4ms attribution is hard for the next reader to reconstruct.

13. TerminalCanvasRendererTestAccess is not needed, and its comment is wrong — PlainAsciiFastPathTest.kt:96-99. isPlainAsciiRun is declared internal, not file-private, and the test is in the same package — VisibleRunLengthTest two classes down calls visibleRunLength directly, which proves it. Delete the wrapper.

14. A test comment contradicts the code — FrameLatencyProbeTest.kt:35 says "Four sub-buckets per octave"; SUB_BITS = 3 gives eight. The Histogram class doc has it right, and its parenthetical about why eight rather than four is the good kind of comment. I checked the indexOf/valueOf round-trip by hand at 8/15/16/17 and the stated bound holds.

15. Settings UI still presents Balanced as the recommended default — PerformanceSettingsSection.kt:187,195: it is first in the dropdown and described as "Good for most users", though the default is now latency. Reordering and swapping the two descriptions keeps the UI honest about the measurement.

16. The PR description is stale. It says draft, not ready to merge, three env vars (BOSSTERM_REDRAW_DEBOUNCE_MS, BOSSTERM_HIGH_VOLUME_DEBOUNCE_MS, BOSSTERM_FAST_TEXT) present for A/B. rg finds none of them, and LATENCY_BASELINE_2026-08-27.md closes with "no flags remain". The body also frames the ASCII/blank work as the whole change, which undersells it — the isAlive() exception storm, the AWT-event coalescing and the visualColToBufferCol fast path are the bigger wins and go unmentioned. Worth rewriting before merge, since the description is what reviewers and git log readers see first.

---

Test coverage

Strong where it counts, and the mutation checks are why I trust it. Gaps, roughly in priority order:

• Nothing pins canBatch/styleMatches. visibleRunLength is extracted and tested, which covers the trim — but the batching predicate is the change with the wider blast radius (blanks joining runs across colour/weight boundaries), and it is covered only by the pixel diff. The block at :1092-1101 is pure given (analysis, style flags, batchText.isEmpty()); extracting a canJoinBatch(...) alongside visibleRunLength, for the same stated reason, would make item 4 testable.
• ColumnConversionFastPathTest never exercises visualCol > width. Both the identity clamp and the wrapLines call shape (visualColToBufferCol(line, terminalWidth, line.length()), i.e. a visual column past the line) go untested. I convinced myself the two paths agree there, but that is the exact call site the 10% claim rests on.
• The isAlive() TTL and death latch are untested. PlatformServices.desktop.kt is awkward to test, but the caching is a pure function of (knownDead, cachedAlive, checkedAt, now) and could be lifted.
• DebugDataCollectorTrimTest is single-threaded, while the drift it guards is concurrent — the doc comment itself says the trim runs on the PTY reader thread. A two-thread record/clear stress would pin the invariant the tests describe. The trim resync-on-null-poll is a nice touch.

TerminalLineWriteModelTest being committed as the harness for work deliberately not in this PR, with the reasoning for not shipping the rope walk written down, is exactly right — as is calling out that the first ColumnConversionFastPathTest passed against a deliberately broken build.

Nothing here blocks on its own. Items 9 and 3 are the two I would fix before the next measurement run, and 16 before merge.