perf: prototype highlighting in a Bun Worker - #755
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR adds reproducible investigations of windowed Pierre highlighting and Bun Worker offload, including compact and transferable highlight encodings, benchmarks, documentation, and proposed upstream patches.
Confidence Score: 4/5The patch artifacts need their inconsistent window-flag references fixed before merging; the direct environment access is additional non-blocking cleanup. Both supplied Pierre patches reference an undeclared identifier in the renderer's unconditional setup path, making the dist patch fail at runtime and the upstream patch fail typechecking. Files Needing Attention: patches/@pierre%2Fdiffs@1.2.2.patch, patches/pierre-upstream-windowed-highlight.patch, benchmarks/highlight-worker-offload.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant Main as Benchmark main thread
participant Worker as Bun Worker
participant Pierre as Pierre/Shiki
Main->>Worker: postMessage(diff metadata, format)
Worker->>Pierre: renderDiffWithHighlighter()
Pierre-->>Worker: highlighted HAST
Worker->>Worker: encode HAST/compact/columnar
Worker-->>Main: reply (transfer buffers when columnar)
Main->>Main: lazily rebuild viewport
Main->>Main: compare rendered spans
Prompt To Fix All With AI### Issue 1
patches/@pierre%2Fdiffs@1.2.2.patch:25
**Window flag name breaks patches**
When either patch is applied, the renderer declares `isHighlightedWindow` but unconditionally reads the undefined `isWindowedHighlight` identifier. The published-dist patch therefore throws on every render, while the upstream TypeScript patch fails typechecking, preventing the documented windowed benchmark from running.
### Issue 2
benchmarks/highlight-worker-offload.ts:42-47
**Environment values bypass validation**
The new benchmark reads `LINES`, `REPEATS`, and `VIEWPORT_ROWS` directly from `process.env` instead of the repository-required Varlock mechanism. This bypasses type-safe configuration validation and lets malformed values propagate as `NaN` or fail later without configuration-level diagnostics.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "perf: make the worker payload cost indep..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - const shouldGroupAll = !forcePlainText && !diff.isPartial; | ||
| - const expandedHunksForIteration = forcePlainText ? expandedHunks : void 0; | ||
| + const shouldGroupAll = !forcePlainText && !isWindowedHighlight && !diff.isPartial; | ||
| + const expandedHunksForIteration = forcePlainText || isHighlightedWindow ? expandedHunks : void 0; |
There was a problem hiding this comment.
When either patch is applied, the renderer declares isHighlightedWindow but unconditionally reads the undefined isWindowedHighlight identifier. The published-dist patch therefore throws on every render, while the upstream TypeScript patch fails typechecking, preventing the documented windowed benchmark from running.
Prompt To Fix With AI
This is a comment left during a code review.
Path: patches/@pierre%2Fdiffs@1.2.2.patch
Line: 25
Comment:
**Window flag name breaks patches**
When either patch is applied, the renderer declares `isHighlightedWindow` but unconditionally reads the undefined `isWindowedHighlight` identifier. The published-dist patch therefore throws on every render, while the upstream TypeScript patch fails typechecking, preventing the documented windowed benchmark from running.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
This landed on the wrong PR — the patch files moved to #756 when this one was narrowed to the worker only, which is why GitHub marks the thread outdated. Nothing in #755 touches Pierre's renderer.
Answered on the equivalent thread there: #756 (comment)
Short version: not a defect. isWindowedHighlight is pre-existing upstream code backing the already-shipped forcePlainText windowing feature; the patch adds isHighlightedWindow as a separate opt-in flag beside it. Verified with tsc --noEmit exit 0 and Pierre's own suite at 1515 pass / 0 fail against the patched source, plus a live windowed render against the dist patch. The near-identical names are a real readability problem though, and #756 now records the rename as an open question before it goes upstream.
Generated by Claude Code
| const SIZES = (process.env.LINES ?? "2000,8000,30000").split(",").map(Number); | ||
| const REPEATS = Number(process.env.REPEATS ?? 5); | ||
| const TAB_WIDTH = 4; | ||
| // A terminal draws tens of rows whatever the file size, so this is what a windowed consumer needs | ||
| // rebuilt before the next paint. | ||
| const VIEWPORT_ROWS = Number(process.env.VIEWPORT_ROWS ?? 60); |
There was a problem hiding this comment.
The new benchmark reads LINES, REPEATS, and VIEWPORT_ROWS directly from process.env instead of the repository-required Varlock mechanism. This bypasses type-safe configuration validation and lets malformed values propagate as NaN or fail later without configuration-level diagnostics.
Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: benchmarks/highlight-worker-offload.ts
Line: 42-47
Comment:
**Environment values bypass validation**
The new benchmark reads `LINES`, `REPEATS`, and `VIEWPORT_ROWS` directly from `process.env` instead of the repository-required Varlock mechanism. This bypasses type-safe configuration validation and lets malformed values propagate as `NaN` or fail later without configuration-level diagnostics.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 41a50e9, though not for the stated reason.
The Varlock rule doesn't apply here. The cited guidelines.mdc is sourced from modem-dev/modem, a different repository. This repo has no Varlock dependency and no .cursor rules:
$ grep -ril varlock . --exclude-dir=node_modules --exclude-dir=.git # no output
$ ls -a .cursor # no such directory
Benchmarks here read the environment directly, and the pattern flagged is the existing convention:
benchmarks/run.ts:38: samples: Number(process.env.HUNK_BENCHMARK_SAMPLES ?? 3),
benchmarks/run.ts:41: includeHuge: process.env.HUNK_BENCH_INCLUDE_HUGE === "1",
But the NaN concern underneath it is real, and worse here than elsewhere: HUNK_BENCH_LINES=abc would have produced NaN, and the benchmark would have run to completion measuring nothing rather than failing. For a script whose entire output is numbers people make decisions from, silently measuring nothing is the bad failure mode.
So the values are now validated and fail with the offending input:
$ HUNK_BENCH_REPEATS=abc bun run benchmarks/highlight-worker-offload.ts
HUNK_BENCH_REPEATS must be a positive integer, got "abc"
I also renamed them to the HUNK_BENCH_ prefix the other benchmarks use — LINES in particular was generic enough to collide with something unrelated in a shell. Documented in benchmarks/README.md, and a run with HUNK_BENCH_LINES=1500 HUNK_BENCH_REPEATS=1 HUNK_BENCH_VIEWPORT_ROWS=40 produces the expected 40-row rebuilds with the equivalence checks still passing.
Generated by Claude Code
There was a problem hiding this comment.
Fair point on Varlock — that rule belongs to a different repo and shouldn't have been applied here. Acknowledged and noted.
The fix addresses exactly the right concern: silent NaN propagation in a benchmark whose entire purpose is producing numbers people act on is the worst failure mode. Explicit validation with a clear error message, the HUNK_BENCH_ prefix for namespace safety, and documentation in the README is the correct resolution. Nothing more needed here.
A large added file freezes the terminal for as long as Pierre's whole-file highlight call takes: about 780ms at 8000 lines, about three seconds at 30000. Pierre cannot be asked for less than a whole file, so this takes the other route and moves the call off the thread that paints. Offloading alone is not enough. Posting Pierre's HAST back is 20.8MiB for a 30k-line file and still stalls the main thread for ~130ms while it deserializes, so the win decays as files grow. Three changes remove that scaling: Send less. Hunk's flattener reads three things per token, so send those with colors interned into a per-file palette of about 11 entries. Rebuild lazily. A terminal draws tens of rows whatever the file holds, so rebuild one viewport on arrival and the rest while scrolling. Transfer instead of clone. What remained was structured clone of an object graph of a million small arrays, which happens before any of our code runs. A columnar shape of one text blob plus flat Int32Arrays goes over in postMessage's transfer list instead. Together the stall stops tracking file size: single-digit milliseconds across 2k, 8k and 30k lines, against a main thread going 182ms to 2878ms, and wall time within noise of the main-thread render. Both wire shapes round trip through the real worker in the equivalence check and reproduce Pierre's HAST spans exactly through buildSplitRows. Workers survive bun build --compile, but only as an additional entrypoint referenced as ./worker.js; ./worker.ts and ./worker both work from source and fail compiled. Nothing in src changes; integration notes are in the doc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgbAM65ZsLbsjgqZS8Dtav
1d0338f to
d66caa9
Compare
benvinegar
changed the title
Review flagged that LINES, REPEATS and VIEWPORT_ROWS were read straight off process.env, so a malformed value became NaN and produced a run that silently measured nothing. Validate them, failing with the offending input, and rename them to the HUNK_BENCH_ prefix the rest of the benchmarks use. LINES in particular was generic enough to collide with an unrelated variable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgbAM65ZsLbsjgqZS8Dtav
2 participants
Investigation into moving Pierre's syntax highlighting off the thread that paints, so a large added file stops freezing the terminal.
Nothing in
src/changes. This lands a benchmark, a worker, and findings. It is the evidence for a decision, not the decision.The problem
renderDiffWithHighlighterhighlights a whole file in one uninterruptible call. Hunk already schedules those calls on a timer so they don't starve each other, but that does nothing about the length of any single call: ~780ms at 8000 added lines, ~2.9s at 30,000. Hunk makes it worse on purpose —sourceBackedHighlight.tsgrafts full source onto partial diffs so grammar state is right, converting a cheap per-hunk render into a whole-file one.Pierre cannot be asked for less than a whole file, so this takes the other route: keep the call, move it off the main thread. That is what Pierre itself does for the browser.
Result
Worst main-thread stall — measured with a 1ms interval recording the largest gap between ticks, median of 5 repetitions, ~2ms idle floor:
The stall stops scaling with file size. Wall time lands within noise of the main-thread render, so this isn't bought with CPU.
Treat the small numbers as an order of magnitude, not a measurement — the 30k cell has read between 5ms and 24ms across runs on a shared machine, and the baseline moves by hundreds of ms too. The claim is "single-digit to low-tens of ms, and not growing with the file", against a baseline that grows to seconds.
Offloading alone wasn't enough
Posting Pierre's HAST back is 20.8MiB at 30k lines and still stalls ~130ms deserializing, so the win decays as files grow — the wrong direction. Three compounding changes fix that:
Int32Arrays — handed over inpostMessage's transfer list. 32ms → 6ms.They only work together: without lazy rebuild, transferring relocates the cost; without transferring, lazy rebuild leaves deserialization as a floor.
Correctness
Both wire shapes round trip through the real worker — not a local copy of the encoder — and reproduce Pierre's HAST spans exactly through Hunk's own
buildSplitRows: 818 spans on a rewrite fixture (word-diff emphasis), 13,750 on a fully visible new file.Compiled-binary constraint
Workers do survive
bun build --compile, with two footguns:ModuleNotFound resolving "/$bunfs/root/worker.ts"..js. Bun resolves./worker.jsto the TS file from source and to the compiled entrypoint. Both./worker.tsand./workerwork from source and fail compiled — a failure that only appears in a release build.Review history worth knowing
A code review found the first round of numbers were measured with the rebuild outside the timed region, making the compact path look 3–6x better than it was. Three more defects came with it: the equivalence check ran a local copy of the encoder rather than the worker's (and the two had already drifted), single noisy samples were reported as precise, and reply-shape ordering always favored compact. All fixed before the optimization work above — which is why the benchmark now repeats, alternates order, reports ranges, and shares one encoder module.
Not done
Integration, and it's larger than the benchmark suggests:
HighlightedDiffCodeis HAST andflattenHighlightedLinecaches spans in aWeakMapkeyed on node identity;aliasHighlightedContextLinesandremapSourceBackedHighlightmanipulate HAST arrays; the worker needs Hunk's content-addressed custom themes;loadHighlightedSourceLineswould become the remaining stall. Listed in the doc.Also unmeasured: binary-size delta against Hunk's real build, dropping word-diff above a size threshold, and
shiki-jsvsshiki-wasm.Related
A second approach — patching Pierre to highlight a row window instead of a whole file — was prototyped alongside this and is in a separate PR. It reaches ~28ms per window at 8000 lines, proportional to window size, with the main thread still doing all the work. It's worth sending upstream on its own merits but is not Hunk's answer.
Verification
typecheck,lint,format:check,knipclean.bun run testunchanged from a clean-tree baseline — 2704 tests, same 26 pre-existing environment failures (session/daemon/CLI tests needing process spawning) on bothmainand this branch.Reproduce:
bun run bench:highlight-worker-offload.