-
Notifications
You must be signed in to change notification settings - Fork 106
perf(levm): use signed remaining gas, improving gas cost calculation perf #4684
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 Block Execution Results Comparison Against Main
|
// EIP-2200 | ||
let gas_left = self.current_call_frame.gas_remaining; | ||
if gas_left <= SSTORE_STIPEND { | ||
if gas_left as u64 <= SSTORE_STIPEND { |
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.
Maybe just change SSTORE_STIPEND
to be i64
.
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 will work for real use cases but gas should theoretically be 64 bits according to the protocol, so we have to decide whether to stop supporting cases in which gas is larger than 63 bits or not.
Agree, thats why the pr is in draft yet, we need to. decide for example whether we enforce a maximum gas limit below i64::max |
EIP-7825 comes in Fusaka, limiting the gas limit per transaction to 2^24. I think it's safe to use |
Yeah that's right, just have to make sure to skip related tests and comment why. We should also add a comment above the |
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
|
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.
Good
What if we change the rest of the gas variables to i64
too? Would that make more sense instead of converting types every time? Just wanted to know your opinion, it's not necessary to do it in this PR
we would need to see if its worth it |
Motivation
Description
Closes #issue_number