feat: add extra sstore benchmark cases#1774
feat: add extra sstore benchmark cases#1774LouisTsai-Csie wants to merge 5 commits intoethereum:forks/amsterdamfrom
Conversation
LouisTsai-Csie
added
A-test-benchmark
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #1774 +/- ##
================================================
Coverage 86.33% 86.33%
================================================
Files 538 538
Lines 34557 34557
Branches 3222 3222
================================================
Hits 29835 29835
Misses 4148 4148
Partials 574 574
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:
|
spencer-tb
left a comment
spencer-tb
left a comment
There was a problem hiding this comment.
Thanks some comments! :)
LouisTsai-Csie
force-pushed
the
feat/extra-sstore-benchmark
branch
2 times, most recently
from
1072748 to
0f4e7eb
Compare
|
@LouisTsai-Csie @marioevz should we merge this such that we can start taking metrics for repricings? |
LouisTsai-Csie
force-pushed
the
feat/extra-sstore-benchmark
branch
3 times, most recently
from
737f7e3 to
e95e45c
Compare
jochem-brouwer
left a comment
jochem-brouwer
left a comment
There was a problem hiding this comment.
Some comments, I'll review the final test for SLOAD tomorrow 😄 👍
LouisTsai-Csie
force-pushed
the
feat/extra-sstore-benchmark
branch
from
e95e45c to
2fd3875
Compare
CPerezz
left a comment
CPerezz
left a comment
There was a problem hiding this comment.
PR tittle should change to: feat(benchmark): add extra sstore benchmark cases
Some minor things here and there. But looks good overall!
| Benchmark SSTORE instruction with various configurations. | ||
|
|
||
| Variants: | ||
| - use_access_list: Warm storage slots via access list | ||
| - sloads_before_sstore: Number of SLOADs per slot before SSTORE | ||
| - num_contracts: Number of contract instances (cold storage writes) | ||
| - initial_value/write_value: Storage transitions | ||
| (zero_to_zero, zero_to_nonzero, nonzero_to_zero, nonzero_to_nonzero) |
There was a problem hiding this comment.
This might be copy-paste? But the test is about SLOADs and the docstring mentions SSTOREs
| contract_gas_limit = base_gas_per_contract | ||
| if contract_idx == len(txs) - 1: | ||
| contract_gas_limit += gas_remainder |
There was a problem hiding this comment.
txs is being built, so this is always False initially.
At the point of this check, txs is being populated in the loop, so len(txs) - 1 will always be less than contract_idx on the first iterations. The condition should be:
if contract_idx == num_contracts - 1: is what we should have??
| if storage_keys_set: | ||
| slots = {i * incrementer for i in range(num_slots)} |
There was a problem hiding this comment.
If incrementer = 0, all slots collapse to {0}, meaning only 1 slot is accessed regardless of num_slots. Is this the intended behaviour?
| start_marker = 10 | ||
| end_marker = 30 + (2 if sloads_before_sstore else 0) |
There was a problem hiding this comment.
Can we give const values or add a comment to avoid magic numbers in the test?
| benchmark_test( | ||
| pre=pre, | ||
| blocks=[Block(txs=[tx])], | ||
| skip_gas_used_validation=True, | ||
| ) |
There was a problem hiding this comment.
Wouldn't it be healthy to make sure we are indeed reading something? So basically, add a minimal check here? It won't even run on the measured block so should not be an issue.
| base_gas_per_contract = min( | ||
| tx_gas_limit, gas_benchmark_values // num_contracts | ||
| ) | ||
| gas_remainder = tx_gas_limit % num_contracts |
There was a problem hiding this comment.
Should we do something like this?
| gas_remainder = tx_gas_limit % num_contracts | |
| if gas_benchmark_values // num_contracts <= tx_gas_limit: | |
| base_gas_per_contract = gas_benchmark_values // num_contracts | |
| gas_remainder = gas_benchmark_values % num_contracts | |
| else: | |
| base_gas_per_contract = tx_gas_limit | |
| gas_remainder = 0 |
| return setup + loop + cleanup | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("slot_count", [50, 100]) |
There was a problem hiding this comment.
I think my main issue with these tests is that they hard-code a number of slots to be accessed and completely disregard gas usage, which makes these tests incompatible with the rest of the benchmarking tests IMO.
Is this a specific requirement for bloatnet or can we scrap this parameter and make the slot count a function of the gas to be used?
| benchmark_test: BenchmarkTestFiller, | ||
| pre: Alloc, | ||
| tx_gas_limit: int, | ||
| gas_benchmark_values: int, |
There was a problem hiding this comment.
| gas_benchmark_values: int, | |
| gas_benchmark_value: int, |
| pytest.param(0, 0xDEADBEEF, id="zero_to_nonzero"), | ||
| pytest.param(0xDEADBEEF, 0, id="nonzero_to_zero"), | ||
| pytest.param(0xDEADBEEF, 0xBEEFBEEF, id="nonzero_to_diff"), | ||
| pytest.param(0xDEADBEEF, 0xBEEFBEEF, id="nonzero_to_same"), |
There was a problem hiding this comment.
This writes a different value, not the same value
| tx_gas_limit: int, | ||
| ) -> None: | ||
| """ | ||
| Benchmark SSTORE instruction with various configurations. |
There was a problem hiding this comment.
| Benchmark SSTORE instruction with various configurations. | |
| Benchmark SLOAD instruction with various configurations. |
7 participants
🗒️ Description
Add extra SSTORE benchmark cases, more details linked in the issue #1755 .
🔗 Related Issues or PRs
Issue #1755
✅ 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