-
Notifications
You must be signed in to change notification settings - Fork 376
feat(benchmark): support --fixed-opcode-count flag and tests
#1747
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
feat(benchmark): support --fixed-opcode-count flag and tests
#1747
Conversation
9097d5f to
f46820c
Compare
|
If the |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/osaka #1747 +/- ##
============================================
Coverage 86.08% 86.08%
============================================
Files 743 743
Lines 44072 44072
Branches 3891 3891
============================================
Hits 37938 37938
Misses 5656 5656
Partials 478 478
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jsign Yes it would by default use the same behavior as it is now. But if the flag being specified, it would switch to the new fixed opcode count scenario. Do you think this is straightforward, or how could we improve the workflow here? Thanks |
I think we'll be interested in both styles, one for worst-case block gas limit and the other for regression-like analysis as planned. I think this optional flag and defaulting to worst-case-gas-limit is quite good, so sgtm! |
marioevz
left a comment
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 general looks good to me, but I left a couple of comments I feel we should address.
packages/testing/src/execution_testing/cli/pytest_commands/plugins/shared/benchmarking.py
Outdated
Show resolved
Hide resolved
packages/testing/src/execution_testing/cli/pytest_commands/plugins/shared/benchmarking.py
Show resolved
Hide resolved
| dest="fixed_opcode_count", | ||
| type=str, | ||
| default=None, | ||
| help="Specify fixed opcode counts for benchmark tests as a comma-separated list.", |
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.
IIUC, this value is times one thousand, so we should specify that here.
spencer-tb
left a comment
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.
Thanks. Added some comments.
packages/testing/src/execution_testing/cli/pytest_commands/plugins/shared/benchmarking.py
Outdated
Show resolved
Hide resolved
packages/testing/src/execution_testing/cli/pytest_commands/plugins/shared/benchmarking.py
Show resolved
Hide resolved
f46820c to
92793f3
Compare
|
Thanks for the review @marioevz , @spencer-tb . I update according to the comment, and i am ready for second review I receive some feedback from Kamil. For gas repricing, we only want to run a specific test (only one parameter combination is enough). So i add extract filter logic in the Example: @pytest.mark.repricing(
size=1024 * 1024,
non_zero_data=True,
zeros_topic=False,
fixed_offset=True,
)
@pytest.mark.parametrize(
"size,non_zero_data",
)
@pytest.mark.parametrize(
"zeros_topic",
)
@pytest.mark.parametrize("fixed_offset")
def test_log(...)
...This is flexible, for normal scenario we could simply label the marker without configuring parameter: @pytest.mark.repricing
def test_codesize(
benchmark_test: BenchmarkTestFiller,
) -> None:
"""Benchmark CODESIZE instruction."""
benchmark_test(
code_generator=ExtCallGenerator(attack_block=Op.CODESIZE),
) |
|
I will check the existing benchmark tests to see if we can convert the existing tests to use this "opcode count" metric in it. This would avoid adding this CLI option and would instead add this option to the tests for maintainability of the framework (vs. maintainability of the tests). |
marioevz
left a comment
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, thanks for the changes!
Branch needs a rebase and then we can merge.
| if self.fixed_opcode_count is not None: | ||
| max_iterations = min(max_iterations, 1000) | ||
|
|
||
| print(f"max_iterations: {max_iterations}") |
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 print could be a bit annoying, we should remove IMO.
92793f3 to
fa69c08
Compare
|
Hi @jochem-brouwer , currently i reuse the
A low hanging fruit would be convert the existing tests in this format. But also we could find other ways for this feature. |
|
I don't want to block this as LGTM from myside. |
🗒️ Description
Fixed Opcode Count Benchmark
This update introduces a fixed opcode count benchmark scenario.
A new flag,
--fixed-opcode-count, and a new test marker,gas_ref, have been added. Only tests marked withgas_refsupport the fixed opcode count feature.Example command:
Technical Notes
--fixed-opcode-count, only gas repricing reference tests will be executed.--fixed-opcode-countcommand, the gas limit would be configured as 1000M gas limit by default. Manually configure the--gas-benchmark-valueswill trigger an error.Benchmark Pattern
setup,attack_block) from the benchmark test wrapper and generate a contract that iterates the attack_block 1000 times.fixed-opcode-counttimes.Example
Setting
--fixed-opcode-count 200means executing the opcode 200 × 1000 = 200,000 times in total.The first contract runs 1000 iterations per call, while the second contract repeats those calls 200 times.
🔗 Related Issues or PRs
issue #1604
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture