-
Notifications
You must be signed in to change notification settings - Fork 771
Add test to re-execute specified range of mainnet C-Chain blocks #4019
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
base: master
Are you sure you want to change the base?
Conversation
- name: Download Previous Benchmark Result | ||
uses: actions/cache@v4 | ||
with: | ||
path: ./cache | ||
key: ${{ runner.os }}-reexecute-cchain-range-benchmark.json | ||
- name: Compare Benchmark Results | ||
uses: benchmark-action/github-action-benchmark@v1 | ||
with: | ||
tool: 'go' | ||
output-file-path: $GITHUB_WORKSPACE/reexecution-data/reexecute-cchain-range.txt | ||
external-data-json-path: ./cache/${{ runner.os }}-reexecute-cchain-range-benchmark.json |
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.
Writing to the cache and comparing against the last entry enables a comparison.
This should be changed to write to the cache only on the master branch for comparison and other triggers should compare against the latest baseline set by master
Trying out running on ARC and Blacksmith here: |
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.
I agree with most of Maru's comments, my only real additional question is:
tests/bench/c/vm_test.go
Outdated
func BenchmarkReexecuteRange(b *testing.B) { | ||
benchmarkReexecuteRange(b, sourceBlockDirArg, targetDirArg, startBlockArg, endBlockArg, chanSizeArg, metricsEnabledArg) | ||
} |
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.
Is this correct usage of golang Benchmark
tests? I've never seen a benchmark that didn't utilize b.N
to repeatedly run the benchmark.
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.
This is definitely a bastardization to produce the benchmark output and requires setting -benchtime=1x
. Will document this and make sure it fails loudly. The expectation here is that it is going to overwrite the current-state
and does not handle reverting or creating a new copy of the same thing.
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.
Adding:
require.Equalf(b, 1, b.N, "BenchmarkReexecuteRange expects to run a single iteration because it overwrites the input current-state, but found (b.N=%d)", b.N)
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.
LGTM
Why this should be merged
This PR adds a configurable C-Chain benchmark that can be run locally with Taskfile or via GitHub Action (this PR currently includes triggers for pull requests and manual GH Action, but this should be changed to a cronjob on master and manual GH Action before merge and once done testing).
The test clones a block database and current state database, so that it can execute a range of blocks starting from the state at an arbitrary height N.
The CI workflow copies the data from an S3 Bucket in the Ava Labs Experimental Account in us-east-2 and requires AWS credentials to be provided. It can also be used with local file system directory paths for use on Snoopy/Linus (dedicated machines with large SSDs used for Firewood testing).
How this works
Constructs an EVM instance mimicking the params passed in from AvalancheGo including passing in a metrics registry with the expected prefix and chain labels attached to it, so that it can be used directly with existing Grafana Dashboards used with tmpnet rather than needing to maintain an alternative without the prefixes/labels.
How this was tested
This has been tested by running in CI and running through the steps documented in the README. To review this PR, please walk through the steps in the benchmark README.
Need to be documented in RELEASES.md?
No.
Recommended Changes Before Merge
Recommended Fast Follow Ups to this PR
gh-pages
branch or a separate repo (demo of what this will look like on my fork of Canoto herePotential Future Work