GODRIVER-4154 Fix correctness bugs in the benchmark harness. - #2621
Open
matthewdale wants to merge 1 commit into
Open
matthewdale wants to merge 1 commit into
matthewdale wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
review-priority-normal
Contributor
API Change ReportNo changes found! |
matthewdale
force-pushed
the
godriver4154-fix-bench
branch
from
September 18, 2026 03:51
c01c887 to
acda99e
Compare
Contributor
🧪 Performance ResultsCommit SHA: 9b39f80The following benchmark tests for version 6aaddab6430f68000743dde6 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
matthewdale
force-pushed
the
godriver4154-fix-bench
branch
2 times, most recently
from
September 18, 2026 21:41
2ff1431 to
d80589e
Compare
matthewdale
force-pushed
the
godriver4154-fix-bench
branch
from
September 19, 2026 00:43
d80589e to
9b39f80
Compare
matthewdale
marked this pull request as ready for review
September 19, 2026 00:48
matthewdale
requested review from
ayesha-ilm and
qingyang-hu
and
a lite review from Copilot
September 19, 2026 00:48
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are internally consistent and address the described benchmark harness issues without identified blockers.
Review effort: Lite
Findings: None
What changed in this PR
This PR fixes benchmark harness correctness and makes fixture setup work consistently for both individual benchmarks and the full suite.
Changes:
- Corrects single-insert fixture selection and benchmark timing.
- Simplifies fixture paths and provisions test data in
TestMain. - Fixes benchmark Taskfile commands and Ping context cleanup.
| File | Description |
|---|---|
Taskfile.yml |
Updates benchmark task commands. |
internal/cmd/benchmark/benchmark_test.go |
Fixes fixture loading, setup cleanup, timing, and data provisioning. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
GODRIVER-4154
Summary
Bugs fixed:
benchmarkSingleInsertso it use thesourceparameter to load the fixture data instead of hard-coding thesmallDatafile.BenchmarkLargeDocInsertOneused thesmallDatafixture, making the results identical toBenchmarkSmallDocInsertOne.ResetTimerbefore starting the benchmark loop inbenchmarkSingleInsertso the benchmark doesn't try to measure the test setup phase.testdataDirfunc with a relative path constant.setupBenchnever calls theCancelFuncfor the Context used toPing.Improvements:
TestMainso they're downloaded independent of how the benchmarks are run.Background & Motivation
While experimenting with ways to reduce noise in the PR benchmark data, Claude found a number of bugs in the existing benchmark code.