docs: refresh upstream performance comparison - #38
Merged
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 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 PR adds controlled upstream-versus-fork benchmark scripts, statistical analysis, committed benchmark evidence, and revised comparison and README documentation. ChangesUpstream Benchmark Comparison
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BenchmarkRunner
participant ForkBinary
participant UpstreamBinary
participant ResultAnalyzer
BenchmarkRunner->>ForkBinary: run pinned benchmark samples
BenchmarkRunner->>UpstreamBinary: run paired benchmark samples
ForkBinary-->>BenchmarkRunner: return fork timing output
UpstreamBinary-->>BenchmarkRunner: return upstream timing output
BenchmarkRunner->>ResultAnalyzer: provide result logs and order TSV
ResultAnalyzer-->>BenchmarkRunner: print reductions, intervals, and diagnostics
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@STOCK-COMPARISON.md`:
- Around line 55-58: Clarify the point-estimate description in the analysis
section: distinguish the geometric mean time ratio, calculated as
exp(mean(log(fork_time / upstream_time))), from the reported percentage
reduction, calculated as 100 * (1 - exp(mean(log_ratio))). Ensure the table’s
reduction metric is explicitly identified as the latter.
In `@tools/run_upstream_benchmark.sh`:
- Around line 80-85: Update the command-recording printf statements in the
benchmark setup and fork build sections to include every argument actually
passed to go test, including the -o output path and package (.). Keep
commands.log aligned exactly with the corresponding executed commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 88e03372-1f8f-48f1-99be-8bd0f8add745
⛔ Files ignored due to path filters (5)
benchmarks/upstream-20260717/commands.logis excluded by!**/*.logbenchmarks/upstream-20260717/final-large1-order.tsvis excluded by!**/*.tsvbenchmarks/upstream-20260717/final-order.tsvis excluded by!**/*.tsvbenchmarks/upstream-20260717/pilot-large1-order.tsvis excluded by!**/*.tsvbenchmarks/upstream-20260717/pilot-order.tsvis excluded by!**/*.tsv
📒 Files selected for processing (24)
README.mdSTOCK-COMPARISON.mdbenchmarks/upstream-20260717/README.mdbenchmarks/upstream-20260717/analysis.txtbenchmarks/upstream-20260717/environment-end.txtbenchmarks/upstream-20260717/environment.txtbenchmarks/upstream-20260717/final-build-fork.txtbenchmarks/upstream-20260717/final-build-upstream.txtbenchmarks/upstream-20260717/final-large1-fork.txtbenchmarks/upstream-20260717/final-large1-upstream.txtbenchmarks/upstream-20260717/final-scan-fork.txtbenchmarks/upstream-20260717/final-scan-upstream.txtbenchmarks/upstream-20260717/matchfirst-benchmem-fork.txtbenchmarks/upstream-20260717/pilot-build-fork.txtbenchmarks/upstream-20260717/pilot-build-upstream.txtbenchmarks/upstream-20260717/pilot-large1-fork.txtbenchmarks/upstream-20260717/pilot-large1-upstream.txtbenchmarks/upstream-20260717/pilot-scan-fork.txtbenchmarks/upstream-20260717/pilot-scan-upstream.txtbenchmarks/upstream-20260717/test-fork.txtbenchmarks/upstream-20260717/test-upstream.txttools/analyze_upstream_benchmark.pytools/benchmark-requirements.txttools/run_upstream_benchmark.sh
Greptile SummaryThis is a documentation-and-tooling PR that replaces stale multi-core AMD EPYC benchmark numbers with a new single-core Graviton3 comparison against the upstream library. No library implementation is changed.
Confidence Score: 4/5Safe to merge — no library code is touched and the benchmark tooling is self-contained and well-validated against the committed raw data. The README introduces a "512k-pattern" pattern count that doesn't correspond to any data in this repository (old or new), and uses "linked" to imply a hyperlink that doesn't exist. A reader following the sentence to verify the claim will find nothing. The rest of the PR — the runner, analyzer, pinned requirements, committed raw samples, and STOCK-COMPARISON.md — is internally consistent and methodologically sound. README.md lines 18–19 contain a factual claim that needs correction before the updated documentation accurately represents the repository's benchmark history. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_upstream_benchmark.sh] --> B[check_checkout fork & upstream]
B --> C[Build fork.test & upstream.test]
C --> D[Capture environment.txt]
D --> E[run_primary_stage pilot 10 500ms]
D --> F[run_large_stage pilot 10]
E --> G[run_primary_stage final 31 1s]
F --> G
G --> H[run_large_stage final 31]
H --> I[run_matchfirst_allocation_check]
I --> J[Capture environment-end.txt]
J --> S[analyze_upstream_benchmark.py]
S --> T[read_samples for each endpoint]
T --> U[Paired log-ratios]
U --> V[Percentile bootstrap CI - Bonferroni-adjusted]
V --> W[Print results table]
%%{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[run_upstream_benchmark.sh] --> B[check_checkout fork & upstream]
B --> C[Build fork.test & upstream.test]
C --> D[Capture environment.txt]
D --> E[run_primary_stage pilot 10 500ms]
D --> F[run_large_stage pilot 10]
E --> G[run_primary_stage final 31 1s]
F --> G
G --> H[run_large_stage final 31]
H --> I[run_matchfirst_allocation_check]
I --> J[Capture environment-end.txt]
J --> S[analyze_upstream_benchmark.py]
S --> T[read_samples for each endpoint]
T --> U[Paired log-ratios]
U --> V[Percentile bootstrap CI - Bonferroni-adjusted]
V --> W[Print results table]
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
README.md:18-19
**Unsupported "512k-pattern" claim and broken "linked" reference**
The sentence "In the linked historical 512k-pattern benchmark" contains two problems. First, no benchmark in this repository — old or new — involves 512k patterns; the removed performance section topped out at `BenchmarkTrieBuild/100000-4` (100k patterns). Second, "linked" conventionally implies a hyperlink, but no hyperlink to a historical benchmark exists here; the only link is `(see [performance](#performance))`, which now resolves to the fork-vs-upstream section rather than any comparison with other Go implementations. A reader looking for the 512k-pattern evidence will find nothing. The claim should either reference the correct pattern count (100k) and link explicitly to the external `aho-corasick-benchmark` repo, or be reworded to avoid implying verifiable anchored evidence.
### Issue 2 of 2
tools/run_upstream_benchmark.sh:173-175
**Pilot stage runs build and large benchmarks redundantly**
`pilot_scan_bench='^BenchmarkPub'` matches every public benchmark, including `BenchmarkPubBuild` and `BenchmarkPubLarge_Sorted10k`. Inside `run_primary_arm`, step 1 therefore runs build benchmarks as part of the broad scan selector, and step 2 then runs `$build_bench` again explicitly. Separately, `run_large_stage pilot 10` runs the large benchmark a second time. Both duplications are harmless to final inference (pilot data is excluded), but they make the pilot meaningfully slower than necessary. Narrowing `pilot_scan_bench` to the same four-endpoint selector used in the final stage, or removing the redundant second `run_benchmark` call for the pilot path, would avoid the overlap.
Reviews (1): Last reviewed commit: "docs: refresh upstream performance compa..." | 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 (2 snapshots, latest commit 3a7e932)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3a7e932)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous review (commit 31a3d64)Status: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (30 files)
Reviewed by gpt-5.6-sol · Input: 54.5K · Output: 3.5K · Cached: 238.3K |
![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: baf85980ee
ℹ️ 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".
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
1 participant
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
Results
Across the six primary workloads, the fork reduced time by 30.02% to 98.17%. The README also reports scan MB/s plus
B/opandallocs/op. Allocation traffic is kept separate from retained or peak memory, which this experiment did not measure.Each endpoint uses 31 paired process executions per revision, alternating arm order on one pinned Graviton3 core. The report uses paired log ratios and Bonferroni-adjusted 99.1667% percentile-bootstrap intervals.
Validation
test -z "$(gofmt -l .)"go vet ./...go test -count=1 ./...go vet -tags purego ./...go test -count=1 -tags purego ./...go test -count=1 -race ./...go test -count=1 -gcflags=all=-d=checkptr=2 ./...shellcheck tools/run_upstream_benchmark.shbenchstatcomparisonDocumentation and benchmark tooling only; no library implementation changes.
This adds a reproducible upstream-versus-fork benchmark workflow and refreshes the README with current results. The fork is reported as 30.02%–98.17% faster across six single-core Graviton3 workloads, with throughput and allocation data; library code is unchanged.
Why it was needed
The previous performance comparison was stale and difficult to reproduce consistently. This change fixes the benchmark revisions, inputs, environment, sampling protocol, and statistical analysis while correcting related README information.
How it works
run_upstream_benchmark.shvalidates exact revisions, clean checkouts, and SHA-256 hashes for benchmark inputs.GOMAXPROCS=1and alternating upstream/fork order.analyze_upstream_benchmark.pyparses process blocks, validates sample counts and ordering, then computes paired log-ratio reductions, bootstrap intervals, diagnostics, throughput, and allocation metrics.Key decisions
MatchFirstis omitted from allocation tables where per-operation allocation comparison is not meaningful.Review focus
PASS-delimited process blocks.Files
README.mdSTOCK-COMPARISON.mdtools/run_upstream_benchmark.shtools/analyze_upstream_benchmark.pytools/test_analyze_upstream_benchmark.pytools/benchmark-requirements.txtbenchmarks/upstream-20260717/