You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(conversation): restore the network cost of a Claude tool picture's backward call search - #2668
#2627's backward call search made a warm tool picture over network storage cost more than it did before #2627: 3.5 s against 1.9 s for an 18 MiB transcript over SSHFS. It looks for the start of the line each backward window cuts 64 KiB at a time, and each read is a round trip there. This PR restores that cost. It's split out of #2610 as you suggested and doesn't depend on the shared reader.
Three commits, all in server/conversation.ts, each with a test in server/claude-images.test.ts:
Growing pieces into one scratch buffer. The walk finds a cut line's start in pieces that double, from 64 KiB up to a search window. Each window is twice what the one before it covered. The pieces are read into one buffer per walk (readStreamInto). Which lines are judged, and in which order, is unchanged.
No copy in linesWithSteps. Each piece of a line is searched alone, and so is the window's cut tail joined with the piece's first span characters. A line longer than a window is no longer copied onto the tail. The pattern has no lookarounds and span is its longest match, so what is found is unchanged.
Head reuse. A window begins with the bytes the probe already read of its line, from the scratch buffer, with no step between them and the window's own read.
The warm-page regression you saw
You measured a warm page of 24 at +4–7% on the earlier version. I bisected it with a process-level ABBA, 12 rounds with rotating and reversing order. Median per-round ratio to main, wall / CPU:
version
18 MiB page
125 MiB page
control (main's tree)
0.97 / 0.97
1.00 / 1.01
growing pieces, as first pushed
1.11 / 1.15
1.17 / 1.20
growing pieces + scratch buffer
0.95 / 0.94
1.03 / 1.00
first version + no yield
1.16 / 1.26
1.23 / 1.35
this PR (no yield + scratch buffer)
0.89 / 0.86
0.87 / 0.83
The cause was the growing probe allocating a fresh zeroed buffer for each piece, up to 1 MiB. A line of a MiB cost about 2 MiB of allocation every time a lookup crossed it. The extra setImmediate per window is also gone.
Measurements
Your harness (your gist and fixture, A B B A, 6 processes per block), page15, main 4bc043d against this PR:
page of 24
main
this PR
warm, pooled median (n=168)
470.7 ms
480.6 ms
cold, pooled median (n=12)
1681 ms
1796 ms
Block medians are 489.1, 464.5, 486.9 and 456.1 ms, so the ~30 ms spread between blocks is wider than the gap between sides. Cold disagrees between orders. On 256 KiB lines the walk does main's 132 reads and 48 turns per page, so there's nothing to gain or lose there. 0 nulls of 4320 on both.
Lines of a MiB, in process (process-level ABBA, 12 rounds), median ratio to main [IQR]:
A warm image takes 0.68× main's time at 18 MiB and 0.75× at 125 MiB, and the first image is back to pre-#2627's level. What's left of the warm gap is the whole-second remainder you accepted on #2627: SSHFS gives whole-second change times, so those lookups still check their bytes. Worst /api/health stall is the same on every version: about 1.8 s on the first image (from scanParent and planState, not images) and 0.4–0.5 s warm. All requests 200.
Checks
bun run check fast on 93da2e0: 10886 pass, 0 fail, 41 skip. Typecheck and build ok.
claude-images.test.ts and conversation.test.ts: 235 pass.
Fuzz with 400 rounds each, seeds 2627 and 1–10: no differences against main 4bc043d's module, and none against the plain search.
Dropping the edge re-test (hit = pattern.test(piece) alone) fails both edge tests.
Known limit
As accepted on #2627: a version whose change time has whole-second precision falls back to the search.
… way back to a Claude tool's call
The backward call walk looked for the start of the line each window cuts 64 KiB at a time, and
each window began twice as long as the one before, however far the line had taken the last one.
Over lines of a MiB that was a read per 64 KiB, and on a network home every read is a round trip:
a warm tool picture whose call is 18 MiB back took 108 reads, against 42 before the index.
Each piece back is now twice the one before, up to a search window, and each window twice what
the one before covered. The pieces are read into one buffer the walk keeps: a buffer allocated
for each, up to a MiB, made a warm page of 24 such pictures cost a fifth more CPU than before.
Which lines are judged, and in which order, is unchanged.
Measured: the same lookup takes 63 reads instead of 108.
…g a long one onto the text before it
The search for the lines that name a call tested each piece of a line with the end of the window
before it in front, so text a window's edge cut in two is still found. Joined to a piece of a MiB,
those few characters made the search copy the whole piece first: every line longer than a window
paid a copy of a MiB per window it spans.
The piece is now searched alone, and the cut text in the end of the window before it and the
piece's first characters. What is found is unchanged; a test puts a call's id across a window's
edge.
…inding its line's start read
The piece that finds the start of the line a backward window cuts holds that line's first bytes;
the window read them again from the line's start, so a window of a MiB took a second, short read
for the rest of its line. It now begins with those bytes, still in the walk's buffer, and reads
from where the piece began, in the same step: the piece's read was a step of its own already.
A test puts a call's id across the edge between the piece's bytes and the window's own read.
Measured: a warm tool picture whose call is 18 MiB back over lines of a MiB takes 45 reads
instead of 63 (108 on main). A warm page of 24 such pictures, process-level ABBA against main
over 12 rounds: 0.89x main's wall time at 18 MiB and 0.87x at 125 MiB.
Auto incremental reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Bug Fixes
Improved image lookup for earlier tool calls in long conversations, including cases where tool-call identifiers span internal search boundaries.
Long transcript lines are now searched with fewer read operations, helping keep lookups efficient.
The backward Claude image lookup now expands its search span and reuses bytes read during the probe. Windowed searches use those bytes and retain only a bounded suffix across window edges. New tests cover long transcript lines and tool-call IDs split across search boundaries.
The search reads backward in progressively larger pieces into a reusable buffer and passes probe bytes to the ID and numeric tool_use searches. Window matching uses a bounded suffix. Tests cover long lines, read counts, and split tool-call IDs in main-style and async lookups.
devswha/ovis#2627: Introduced the backward call search that this change optimizes.
devswha/ovis#1935: Added Claude tool-result picture retrieval, which uses the optimized transcript search.
devswha/ovis#2604: Added the synchronous and cooperative asynchronous image scan paths that use the optimized search.
Suggested reviewers:devswha
Merge Risk: Minimal · previous review 93da2
No actionable merge-blocking defect is established for the buffered backward-search change. Merge after normal checks pass.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly identifies the performance change to Claude tool-picture backward call searches. It is specific and related to the main changeset.
Description check
The description explains the problem, implementation, measured results, validation, and known limit. It uses Summary and Checks headings instead of the required Change and Validation headings, and it …
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Description check
Explanation
The description explains the problem, implementation, measured results, validation, and known limit. It uses Summary and Checks headings instead of the required Change and Validation headings, and it does not explicitly list skipped checks, but the required information is mostly present.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between 4bc043d and 93da2e0.
Files selected for processing: 2
server/claude-images.test.ts
server/conversation.ts
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
@devswha a friendly nudge on this one too. It's the back-walk fix on its own, as you asked: level with main on your page15, and about 0.9× main on lines of a MiB. Over SSHFS it brings a warm image back from 3.5 s to 2.4 s (pre-#2627: 1.9 s). It's running on my fork's main with no issues so far. Happy to change anything you'd like.
Thanks @duytbp, this is a clean split of the back-walk fix.
What I did on your branch (maintainer edit, no force push):
Merged current main into it (28ba6f6). There were no conflicts, and nothing in your commits changed.
How it was checked:
The read-count test fails on main's server/conversation.ts (114 reads, limit < 60) and passes on this head. The two edge tests pass on both, as regression guards for the new tail and head logic.
bun test server/claude-images.test.ts server/conversation.test.ts: 243 pass, 0 fail.
bun run check fast on the merged head: typecheck, build and unit tests all ok.
Adversarial review: no findings. It covered scratch-buffer reuse across the numeric-id fallback, hidden-range blanking, multi-file streams, short reads, and matches cut at window and probe edges.
No dependency, script, workflow or network changes.
Thanks @duytbp! Your read-count test fails on main (114 reads) and passes here. Merging.
Merge note: external fix; supply-chain check done; bug reproduced (new test fails on main); omo CLEAR at this head; Codex Terra xhigh reviewed; required checks green; CodeRabbit skipped.
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
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.
Summary
#2627's backward call search made a warm tool picture over network storage cost more than it did before #2627: 3.5 s against 1.9 s for an 18 MiB transcript over SSHFS. It looks for the start of the line each backward window cuts 64 KiB at a time, and each read is a round trip there. This PR restores that cost. It's split out of #2610 as you suggested and doesn't depend on the shared reader.
Three commits, all in
server/conversation.ts, each with a test inserver/claude-images.test.ts:readStreamInto). Which lines are judged, and in which order, is unchanged.linesWithSteps. Each piece of a line is searched alone, and so is the window's cut tail joined with the piece's firstspancharacters. A line longer than a window is no longer copied onto the tail. The pattern has no lookarounds andspanis its longest match, so what is found is unchanged.The warm-page regression you saw
You measured a warm page of 24 at +4–7% on the earlier version. I bisected it with a process-level ABBA, 12 rounds with rotating and reversing order. Median per-round ratio to main, wall / CPU:
The cause was the growing probe allocating a fresh zeroed buffer for each piece, up to 1 MiB. A line of a MiB cost about 2 MiB of allocation every time a lookup crossed it. The extra
setImmediateper window is also gone.Measurements
Your harness (your gist and fixture, A B B A, 6 processes per block),
page15, main 4bc043d against this PR:Block medians are 489.1, 464.5, 486.9 and 456.1 ms, so the ~30 ms spread between blocks is wider than the gap between sides. Cold disagrees between orders. On 256 KiB lines the walk does main's 132 reads and 48 turns per page, so there's nothing to gain or lose there. 0 nulls of 4320 on both.
Lines of a MiB, in process (process-level ABBA, 12 rounds), median ratio to main [IQR]:
HTTP harness (20 repeats, warm gallery, load 12–14), medians in ms:
All 200, all bytes matching.
Reads per lookup (deterministic):
SSHFS (the network lab; 2 runs × 7 repeats at 18 MiB, 2 × 4 at 125 MiB), medians in ms:
A warm image takes 0.68× main's time at 18 MiB and 0.75× at 125 MiB, and the first image is back to pre-#2627's level. What's left of the warm gap is the whole-second remainder you accepted on #2627: SSHFS gives whole-second change times, so those lookups still check their bytes. Worst
/api/healthstall is the same on every version: about 1.8 s on the first image (fromscanParentandplanState, not images) and 0.4–0.5 s warm. All requests 200.Checks
bun run check faston 93da2e0: 10886 pass, 0 fail, 41 skip. Typecheck and build ok.claude-images.test.tsandconversation.test.ts: 235 pass.hit = pattern.test(piece)alone) fails both edge tests.Known limit
As accepted on #2627: a version whose change time has whole-second precision falls back to the search.