-
Notifications
You must be signed in to change notification settings - Fork 130
perf(levm): cache BLOBBASEFEE opcode value
#5288
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
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
Benchmark Block Execution Results Comparison Against Main
|
Arkenan
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.
Nice catch. Left one optional comment.
|
|
||
| let blob_base_fee = | ||
| get_base_fee_per_blob_gas(self.env.block_excess_blob_gas, &self.env.config)?; | ||
| let blob_base_fee = match self.blob_base_fee.get() { |
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.
nit: is this not a bit clearer if we use OnceCell::get_or_init?
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.
Does not work because it can't handle errors. The get_or_try_init method is nightly-only.
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.
Not as much a nit, is needing the blob_base_fee uncommon enough to not just init it with the block, rather than using a OnceCell?
Besides, we already have a &mut self here, so we could just use an option in any case.
SDartayet
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!
Oppen
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.
Ok in principle, before or after merge I'd like to discuss what I commented on. Can be handled later in a separate PR.
**Motivation** Currently in prepare_execution we are, for each transaction, calculating the blob base fee which is consistent across the block. **Description** This was already cached for `BLOBBASEFEE` in #5288, but it could be unified across all users by making part of the Environment. --------- Co-authored-by: Tomás Grüner <[email protected]>
…aclass#5328) **Motivation** Currently in prepare_execution we are, for each transaction, calculating the blob base fee which is consistent across the block. **Description** This was already cached for `BLOBBASEFEE` in lambdaclass#5288, but it could be unified across all users by making part of the Environment. --------- Co-authored-by: Tomás Grüner <[email protected]>
Motivation
Our implementation of
BLOBBASEFEEis slow when compared to any other client.Description
The
BLOBBASEFEEvalue should not change between runs with the same block header and environment, therefore it can be cached and reused.