perf: scale chainSample's budget down at chunk size - #37
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change reduces chain-sampling budgets for small inputs, parameterizes chain-walk limits, adds routing-preservation coverage, and introduces benchmarks for sampling and dispatch regimes. ChangesRouting Sampling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32b404f7f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Greptile SummaryThis PR scales
Confidence Score: 4/5Safe to merge; the routing logic change is correct and well-tested, with no path that silently mis-routes a scan family. The core bifurcation in chainSample is mathematically sound and TestRoutingPreserved pins the expected verdicts on both sides of the budget boundary. Two non-blocking issues exist: a comment inaccuracy about which chunks take the reduced budget (ceiling division at the band ceiling produces 16 384-byte chunks that slip through), and a missing length guard in the new test that would panic rather than fail gracefully if Ibsen.txt were replaced with a shorter file. The chainSampleSmallMax comment block in trie.go (around line 1082) and the ibsen slice in routingpreserve_test.go (around line 45) warrant a second look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["chainSample(input)"] --> B{n < chainSampleSmallMax = 16384?}
B -- Yes --> C["2 windows at n/4 and 3n/4\nmaxSteps=128, maxExc=12"]
B -- No --> D["4 windows at n/8,3n/8,5n/8,7n/8\nmaxSteps=256, maxExc=24"]
C --> E["chainWalk per window"]
D --> E
E --> G{Vote}
G -- "chainBytes>=64 AND chainBytes>=24*exc" --> H["long++"]
G -- "exc>=8 AND chainBytes<6*exc" --> I["short++"]
G -- otherwise --> J["abstain"]
H & I & J --> K["return long, short"]
K --> L{long > short?}
L -- Yes --> M["dual-cursor scan"]
L -- No --> N{short > long?}
N -- Yes --> O["single-cursor scan"]
N -- No --> P["looksDense tie-break"]
P --> Q{dense?}
Q -- Yes --> M
Q -- No --> O
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["chainSample(input)"] --> B{n < chainSampleSmallMax = 16384?}
B -- Yes --> C["2 windows at n/4 and 3n/4\nmaxSteps=128, maxExc=12"]
B -- No --> D["4 windows at n/8,3n/8,5n/8,7n/8\nmaxSteps=256, maxExc=24"]
C --> E["chainWalk per window"]
D --> E
E --> G{Vote}
G -- "chainBytes>=64 AND chainBytes>=24*exc" --> H["long++"]
G -- "exc>=8 AND chainBytes<6*exc" --> I["short++"]
G -- otherwise --> J["abstain"]
H & I & J --> K["return long, short"]
K --> L{long > short?}
L -- Yes --> M["dual-cursor scan"]
L -- No --> N{short > long?}
N -- Yes --> O["single-cursor scan"]
N -- No --> P["looksDense tie-break"]
P --> Q{dense?}
Q -- Yes --> M
Q -- No --> O
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
trie.go:1082-1086
**Comment claims all band chunks take reduced budget — not true for the ceiling-division edge**
The comment states "so every chunk of such a dispatch…takes the reduced budget", but `matchParallel` uses ceiling division (`chunk = (len(input) + p - 1) / p`), so a near-ceiling input such as 131 071 bytes (= `parallelSparseMin - 1`) split across 8 workers produces chunks of exactly 16 384 bytes (`ceil(131071/8) = 16384`). Since the check is `n < chainSampleSmallMax` (strict), those 16 384-byte chunks fall through to the full four-window budget, contradicting the comment.
The behavior is not wrong — using the full budget for a 16 KB chunk is safe and keeps routing correct — but the comment should be amended to say "chunks strictly less than `chainSampleSmallMax`" or the threshold changed to `<=` to cover those 7 edge sizes at the top of the band.
### Issue 2 of 2
routingpreserve_test.go:45-53
**Unguarded 96 KiB slice on `ibsen` panics if the file is replaced**
`ibsen[:96<<10]` is used directly with no prior length check. If `Ibsen.txt` is ever swapped for a shorter fixture (or truncated in CI), the test panics with an index out of range rather than producing a useful failure message. A `t.Fatalf` guard after `mustRead` would make the failure diagnostic instead of a crash.
Reviews (1): Last reviewed commit: "perf: scale chainSample's budget down at..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (3 snapshots, latest commit 0a2b435)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0a2b435)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Fix these issues in Kilo Cloud Files Reviewed (4 files)
Previous review (commit 5771575)Status: 17 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Fix these issues in Kilo Cloud Files Reviewed (3 files)
Previous review (commit 32b404f)Status: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Fix these issues in Kilo Cloud Files Reviewed (3 files)
Reviewed by gpt-5.6-sol · Input: 90K · Output: 6.4K · Cached: 699.7K |
Four 1KB windows under full caps cover a third of a 12KB parallel chunk - 8x the sampling fraction the same code applies to a whole 96KB input - and every worker of a dense-verdict dispatch walks that budget concurrently before scanning its chunk, ~17us of the critical path at 96-127KB. Inputs under chainSampleSmallMax (the largest gate-sampled chunk, parallelSparseMin/8) now sample two windows under halved caps; the cap ratio is preserved so the long/short vote bars are unchanged and only the evidence budget shrinks. Measured (Graviton3, n=24, interleaved executions): dense 96KB -4.8%, 127KB -3.3%, sequential 12KB dense -9.4%, gate-edge separator corpora -5.5/-5.9%; shallow-chain dense input keeps its single-cursor routing at every size (TestRoutingPreserved pins the verdicts on both sides of the budget threshold), sparse and >=16KB-chunk paths unchanged.
aebf57f to
ea55e49
Compare
32b404f to
5771575
Compare
…comment scope fixes - assertRegime evaluates the dispatch at runtime.GOMAXPROCS(0) (the same value Match uses) instead of a hard-coded 64, and resets the timer after passing so the assertion cost stays out of short runs. - TestRoutingPreserved guards the Ibsen fixture length, asserts the trie shape via buildStopByte16Trie (now testing.TB), and reuses concat; spDenseCorpus drops its unused param and reuses concat; spSparseFiller replaced by bytesFill; mustRead replaced by benchReadFile. - chainSampleSmallMax comment scopes the reduced-budget claim to the ceiling-division/overlap edges; chainSample cost bench comment states the solo figure is an uncontended floor, not a bound.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0de9278f72
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Below GOMAXPROCS=8 the dispatcher hands out fewer, larger chunks (96KiB across 4 workers is 24KiB chunks), which take the full sampling budget under row names that promise the reduced one. spTrie now skips the file loudly on constrained runners instead of letting assertRegime pass on p>0 while the rows time the wrong policy regime.
spTrie gated every SP row on GOMAXPROCS>=8, skipping sequential controls and the looksDense/chainSample microbenchmarks that have no worker-count dependency. The gate now lives in spGate8, called only by the groups whose rows document 8-worker chunk geometry (DenseBand, Hetero, FalseStart, Gray). Also drop a change-relative word from the chainSampleSmallMax comment.
1 participant
chainSample's routing vote pays a fixed budget — four 1KB windows, each walking up to 256 chain bytes of dependentfailTrans16loads — regardless of input size. At chunk scale that budget is mis-sized twice over: four windows cover a third of a 12KB parallel chunk (8x the sampling fraction the same code applies to a whole 96KB input), and on a dense-verdict parallel dispatch every worker walks it concurrently before scanning, where the sample's dependent loads run ~5x slower under 8-way LLC contention than solo (~4.9us → ~27us for 8 workers) — about 17us of the dispatch's critical path at 96–127KB.Inputs under
chainSampleSmallMax(parallelSparseMin/8= 16KB, the largest chunk a gate-sampled dispatch hands a worker) now sample two windows (1/4, 3/4 points) under halved per-window caps. The cap ratio is preserved (128/12 = 256/24), so the byte cap still cuts a window off at the same mean excursion length relative todualChainLongMinand the short bar keepsdualChainShortMax— only the evidence budget shrinks, not the vote's calibration. Whole inputs and ≥16KB chunks keep the full budget.Alternatives measured and rejected: forcing one global density verdict onto all chunks captures more (-6.7% at 96KB) but mis-routes shallow-chain dense input +57% (density says dense, chains die at depth 1, single-cursor wins 1.4x — the chunk-local chain vote is what catches this); computing the vote once at the gate is a critical-path no-op (workers sample concurrently, so wall time is
spawn + sample + scaneither way); a sound early-exit inside the walk does not exist (the long vote is non-monotone in the walk state, and certainty bounds land beyond the caps).TestRoutingPreservedpins the dual-vs-single verdict per corpus shape on both sides of the budget threshold, so a future budget change that flips a scan family's routing fails a test rather than a benchmark.Measured (Graviton3, n=12 interleaved executions vs the stack tip): dense concat words -3.9% at 96KB, -2.8% at 127KB; sequential 12KB dense -7.5%; separator corpora at the density gate's edge -5.3/-5.6%; shallow-chain (false-start) input flat-to-better with routing preserved; sparse, hetero, and ≥16KB-chunk controls unchanged.
chainSampleitself: -73% at 12KB, exactly 0 at 16KB+. One noisy row (ibsen-48k, ±2% cv) reports +4.3% on a code path byte-identical in both arms; across five earlier same-code binary pairs it swung ±1.7–4.1% both directions — binary-layout artifact, not mechanism.Follow-up to #32's sampling-guarantee discussion (the deferred half of the per-chunk re-sampling question). Stacked on #36.
This reduces
chainSample’s work for inputs smaller than 16 KB while preserving routing decisions. Small inputs now use two sampling windows with halved per-window limits; full-sized inputs and chunks retain the existing sampling budget.Why it was needed
chainSampleruns during routing decisions for parallel dispatch. Its fixed sampling budget added unnecessary concurrent-sampling overhead for small inputs, especially in dense-verdict workloads. The change targets that cost without changing the cap ratio or shallow-chain routing behavior.How it works
chainSampleSmallMax(16 KB) sample windows near the quarter and three-quarter positions.chainWalknow receives explicit limits and stops when either the step or excursion cap is reached.TestRoutingPreservedcompares dual- versus single-cursor routing across the budget boundary using wordlike and falsestart corpora.chainSamplecosts. Runtime-regime assertions useruntime.GOMAXPROCSto validate that measurements exercise the intended dispatch paths.flowchart LR A[Input size] -->|below 16 KB| B[Reduced chainSample] A -->|16 KB or larger| C[Full chainSample] B -->|two windows, halved caps| D[chainWalk] C -->|four windows, full caps| D D -->|chain/excursion evidence| E[dualWorthwhile routing verdict] classDef changed fill:`#fff4cc`,stroke:`#d4a017`,stroke-width:2px class B,C,D,E changedKey decisions
Review focus
chainWalklimits are enforced consistently and that larger inputs retain the previous budget.Files
trie.gochainWalklimits.routingpreserve_test.gosamplepolicy_bench_test.godualscan_test.gotesting.TB.