perf(spike): measure whether a Rust core pays for itself - #747
Open
benvinegar wants to merge 3 commits into
Open
perf(spike): measure whether a Rust core pays for itself#747benvinegar wants to merge 3 commits into
benvinegar wants to merge 3 commits into
Conversation
Option 1 of the runtime evaluation was to keep the TypeScript surface and move hunk's hot paths into a native module. That only makes sense if the speedup survives the FFI boundary, so this measures it rather than assuming it. `crates/hunk-core` ports the two paths a native core would own first: terminal width measurement (`src/ui/lib/text.ts`) and word-level intraline diff (today folded into Pierre's highlighting pass) on imara-diff's histogram algorithm. Nothing in `src/` imports it; it exists to produce a number. The number is that the computation is 8x faster and the boundary gives most of it back. On a real 66k-line changeset, width measurement runs at 34 ns/line natively against the tuned TypeScript's 272, but only reaches 156 ns/line through bun:ffi -- marshalling is 57% of the batched path, because handing UTF-16 JS strings to a UTF-8 library costs more than the library's work. A 1.7x batched win does not justify a second toolchain and per-platform prebuilts. The generalisation is that a native core pays only when it owns the text rather than borrowing it, which argues for a Rust review core over native helpers under TypeScript ones. Parity is exact on 63,636 real diff lines and a 60-case adversarial unicode corpus. Reaching that required porting string-width's rules rather than approximating them; four real bugs surfaced and are covered by regression tests. The residual 1.79% fuzz mismatch has one known cause, recorded in the module header and README: Extended_Pictographic is approximated by block ranges instead of generated from the UCD, so unassigned emoji-block code points paired with a variation selector measure wide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxpS9muCVHNbSdsA1y8zF8
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
Greptile SummaryThis PR adds an unshipped Rust performance spike for terminal-width measurement and word-level intraline diffs, together with Bun FFI bindings and benchmark/parity harnesses.
Confidence Score: 4/5The intraline FFI binding should handle empty-sided comparisons before merging; the newline-framing contract should also be made explicit or enforced. Empty strings are valid intraline inputs, but the new binding passes their zero-length buffers to an FFI pointer conversion that the same module documents as unsupported, causing those comparisons to fail before reaching the Rust implementation. Files Needing Attention: crates/hunk-core/index.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant Bench as Bun benchmark
participant Binding as index.ts
participant FFI as C ABI
participant Core as Rust core
Bench->>Binding: strings or line arrays
Binding->>Binding: UTF-8 encode and batch
Binding->>FFI: caller-owned buffers
FFI->>Core: width or intraline operation
Core-->>FFI: counts and output spans
FFI-->>Binding: caller-owned output buffer
Binding-->>Bench: widths or grouped spans
Prompt To Fix All With AI### Issue 1
crates/hunk-core/index.ts:122-124
**Empty intraline buffers break FFI**
When either side of an intraline comparison is empty, `intralineSpans` passes a zero-length buffer to `ptr()`, which this module documents as unsupported by Bun FFI, causing valid insertion or deletion comparisons to fail before reaching the Rust implementation. The batch variant has the same failure when one joined side is empty.
### Issue 2
crates/hunk-core/index.ts:159-160
**Batch framing assumes newline-free strings**
The batch APIs join arbitrary strings with an unescaped newline delimiter without documenting or validating that inputs are newline-free. An embedded newline changes native row boundaries, so width batching throws on the unexpected count while intraline batching can misalign pairs, drop rows, or index beyond the result array.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "perf(spike): measure whether a Rust core..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+122
to
+124
| ptr(beforeBytes), | ||
| beforeBytes.byteLength, | ||
| ptr(afterBytes), |
Contributor
There was a problem hiding this comment.
When either side of an intraline comparison is empty, intralineSpans passes a zero-length buffer to ptr(), which this module documents as unsupported by Bun FFI, causing valid insertion or deletion comparisons to fail before reaching the Rust implementation. The batch variant has the same failure when one joined side is empty.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/hunk-core/index.ts
Line: 122-124
Comment:
**Empty intraline buffers break FFI**
When either side of an intraline comparison is empty, `intralineSpans` passes a zero-length buffer to `ptr()`, which this module documents as unsupported by Bun FFI, causing valid insertion or deletion comparisons to fail before reaching the Rust implementation. The batch variant has the same failure when one joined side is empty.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+159
to
+160
| const beforeBytes = encode(before.join("\n")); | ||
| const afterBytes = encode(after.join("\n")); |
Contributor
There was a problem hiding this comment.
The batch APIs join arbitrary strings with an unescaped newline delimiter without documenting or validating that inputs are newline-free. An embedded newline changes native row boundaries, so width batching throws on the unexpected count while intraline batching can misalign pairs, drop rows, or index beyond the result array.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/hunk-core/index.ts
Line: 159-160
Comment:
**Batch framing assumes newline-free strings**
The batch APIs join arbitrary strings with an unescaped newline delimiter without documenting or validating that inputs are newline-free. An embedded newline changes native row boundaries, so width batching throws on the unexpected count while intraline batching can misalign pairs, drop rows, or index beyond the result array.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…e diff engine `hunk --version` costs ~255ms, of which ~13ms is Bun. The compiled binary is no faster than source, so the cost is module evaluation rather than parsing, and one edge into it was `fileLanguage.ts`: a top-level loop calling into `@pierre/diffs` meant importing the module pulled the whole diff engine and its syntax grammars. The startup path imports it through `extensions/apply.ts` on every invocation, to build a two-element Set and register mappings nothing reads until a changeset exists. Registrations are now recorded as plain data and applied at the first lookup. `fileLanguageLookup` owns the only read and the only write of Pierre's process-global extension table, so a mapping cannot be observed before it is applied and the deferral is not detectable. Draining rather than replaying keeps repeat lookups free while letting a late registration land on the next one. `apply.ts` is unchanged: its "extensions never override Hunk's built-ins" policy is decided against Pierre-free data. This does not deliver the startup win on its own, and the measurement should not be read as if it did. `apply.ts` drops from ~125ms to ~7ms, but `--version` only moves ~253ms to ~245ms, because `core/diffFile.ts` still reaches Pierre through the lookup and the VCS and extension graph imports it from `startup.ts`. Closing that second edge means making `startup.ts` defer `getBundledVcsCatalog` and `loadAppBootstrap`, which is a separate change to that function's dependency-injection seams. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxpS9muCVHNbSdsA1y8zF8
`prepareStartupPlan` returns early for `--help`, `--version`, `daemon serve`, the markup commands, `extension-manage`, and `session`, but it statically imported the VCS catalog, the extension bootstrap, and the loading pipeline — and called `getBundledVcsCatalog()` before the first branch. Those reach changeset construction and from there the diff engine and its syntax grammars, so every invocation paid for machinery most commands never touch. The catalog is now resolved through a memoized loader, and the modules used only past the early returns are imported at their use sites, matching the pattern `main.tsx` already uses for `extension-manage`. The dependency-injection defaults for `loadAppBootstrap` and `loadStartupExtensions` move to the same place, so supplying either still short-circuits the import. Run from source, `--version` goes 245ms to 126ms and `hunk session list`, the path agents review through, goes 275ms to 156ms. The compiled binary gains far less: 257ms to 223ms for `--version`, and 285ms to 260ms for `session list`. A trivial compiled binary starts in 11ms, so the remainder is Hunk's own module evaluation rather than a fixed cost of `bun build --compile`; the lazy boundaries hold when Bun resolves modules at runtime and largely collapse once everything is bundled into one file. Closing that gap is a build-configuration question, not another import change, and it is worth roughly 97ms to the artifact users actually run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxpS9muCVHNbSdsA1y8zF8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Option 1 of the runtime evaluation was to keep the TypeScript surface and
move hunk's hot paths into a native module. That only makes sense if the
speedup survives the FFI boundary, so this measures it rather than assuming
it.
crates/hunk-coreports the two paths a native core would own first:terminal width measurement (
src/ui/lib/text.ts) and word-level intralinediff (today folded into Pierre's highlighting pass) on imara-diff's
histogram algorithm. Nothing in
src/imports it; it exists to produce anumber.
The number is that the computation is 8x faster and the boundary gives most
of it back. On a real 66k-line changeset, width measurement runs at 34 ns/line
natively against the tuned TypeScript's 272, but only reaches 156 ns/line
through bun:ffi -- marshalling is 57% of the batched path, because handing
UTF-16 JS strings to a UTF-8 library costs more than the library's work. A
1.7x batched win does not justify a second toolchain and per-platform
prebuilts. The generalisation is that a native core pays only when it owns
the text rather than borrowing it, which argues for a Rust review core over
native helpers under TypeScript ones.
Parity is exact on 63,636 real diff lines and a 60-case adversarial unicode
corpus. Reaching that required porting string-width's rules rather than
approximating them; four real bugs surfaced and are covered by regression
tests. The residual 1.79% fuzz mismatch has one known cause, recorded in the
module header and README: Extended_Pictographic is approximated by block
ranges instead of generated from the UCD, so unassigned emoji-block code
points paired with a variation selector measure wide.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01SxpS9muCVHNbSdsA1y8zF8