-
Notifications
You must be signed in to change notification settings - Fork 840
chore(reexecute/c): remove go bench from benchmark #4640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
decfba5 to
e096dc1
Compare
bd29254 to
1935adf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes the use of go bench from the C-Chain reexecution test, transitioning it from a Go benchmark to a standalone executable. The change enables more flexible metric reporting and aligns the test with its actual usage pattern (single execution rather than iterative benchmarking).
Key Changes:
- Replaced
*testing.Bwith a custombenchmarkToolfor metric collection and reporting - Changed the package from
vmtomainand converted the benchmark function to a standalone executable - Introduced
tests.TestContextthroughout to manage test lifecycle and cleanup operations
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/reexecute/db.go | Updated CreateBlockChanFromLevelDB signature to use tests.TestContext instead of testing.TB and removed cleanup parameter |
| tests/reexecute/c/vm_reexecute.go | Converted from benchmark test to main executable; added custom benchmarkTool for metric reporting and JSON output |
| tests/reexecute/blockexport/main.go | Removed unused cleanup parameter from CreateBlockChanFromLevelDB call |
| scripts/benchmark_cchain_range.sh | Changed from go test to go run and updated output handling to use JSON file directly |
| .github/actions/c-chain-reexecution-benchmark/action.yml | Updated benchmark tool from 'go' to 'customBiggerIsBetter' and changed output file extension to .json |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/benchmark_cchain_range.sh
Outdated
| else | ||
| eval "$cmd" | ||
| fi | ||
| eval "$cmd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this and just run the command now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, removed eval here: 955273e
scripts/benchmark_cchain_range.sh
Outdated
|
|
||
| cmd="go test -timeout=0 -v -benchtime=1x -bench=BenchmarkReexecuteRange -run=^$ github.com/ava-labs/avalanchego/tests/reexecute/c \ | ||
| cmd="go run github.com/ava-labs/avalanchego/tests/reexecute/c \ | ||
| --benchmark-output-file=\"${BENCHMARK_OUTPUT_FILE}\" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should this be handled if the benchmark output file is not defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In vm_reexecute.go, we don't write to a file if the benchmark output filepath is empty, but you're right that if a user currently doesn't set BENCHMARK_OUTPUT_FILE, that we'll have:
go run github.com/ava-labs/avalanchego/tests/reexecute/c --benchmark-output-file= --current-state-dir=...`Changed it so that if a user doesn't define BENCHMARK_OUTPUT_FILE, then --benchmark-output-file isn't set: f682b20
ci: add chaos test job chore: nits chore: add nix installation step chore: configure AWS credentials chore: remove inputs chore: add perms chore: nit chore: nit chore: extend wait time chore: nits chore: nit chore: nit chore: Create shared `evm` module (#4690) chore(reexecute/c): remove go bench from benchmark (#4640) chore: nits fix: MAX_WAIT_TIME chore: nit chore: extend wait times chore: log errs chore: stdout tail chore: nit chore: nits chore: nits ci: improve workflow chore: nits chore: nits chore: nits chore: lint
Why this should be merged
As part of #4508, this PR removes usage of
go benchfrom the reexecution test. This offers the following benefits:addResult()on the new benchmark tool.How this works
*testing.B./scripts/benchmark_cchain_range.shgo benchHow this was tested
CI + ran
task reexecute-cchain-range START_BLOCK=1 END_BLOCK=100_000 BENCHMARK_OUTPUT_FILE=<INSERT_CUSTOM_PATH_HERE>locally and got the following:[ { "name": "BenchmarkReexecuteRange/[1,100000]-Config-default-Runner-dev - mgas/s", "value": "254.90228109633716", "unit": "mgas/s" } ]Furthermore, results are logged to the terminal as follows:
To test that benchmark results display correctly on the GAB dashboard, I created a sample benchmark (https://github.com/RodrigoVillar/sample-reexecution-custom-benchmark/blob/main/main.go) which is a copy-and-paste of the benchmarking code in this PR and plotted some points into my own dashboard. The results of the sample benchmark can be seen below:
Attached is a link to the sample benchmark dashboard: https://rodrigovillar.github.io/sample-reexecution-custom-benchmark/dev/bench/
Need to be documented in RELEASES.md?
No
TODOs after merging
gh-pagesbranch of data points which usego bench