Skip to content
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

Remove SPEC generic usage in gas cost calculation code #1220

Closed
DaniPopes opened this issue Mar 24, 2024 · 2 comments · Fixed by #1243
Closed

Remove SPEC generic usage in gas cost calculation code #1220

DaniPopes opened this issue Mar 24, 2024 · 2 comments · Fixed by #1243
Labels
good first issue Good for newcomers refactor Refactor of the code

Comments

@DaniPopes
Copy link
Collaborator

Usage of the SPEC generic is unnecessary in gas calculation code as the SpecId is already known at compile-time in the instructions and these functions are marked with #[inline], so they would get optimized the same way.

By turning the SPEC generic into a SpecId parameter:

  1. most gas calculation functions can be made const, allowing usage in compile-time gas tables and outside of SPEC-generic code
  2. helps a bit with compile times by reducing monomorphization bloat

SPEC::enabled(X) becomes SpecId::enabled(spec, X).

This likely applies to more places in revm-interpreter, but I haven't looked too deeply into it, and I'm confident that gas calculation is a good starting place.

Examples:

  • Gas::set_final_refund (cannot be made const due to &mut, rest still applies):
    pub fn set_final_refund<SPEC: Spec>(&mut self) {
    let max_refund_quotient = if SPEC::enabled(LONDON) { 5 } else { 2 };
    self.refunded = (self.refunded() as u64).min(self.spend() / max_refund_quotient) as i64;
    }
  • gas::call_gas:
    pub fn call_gas<SPEC: Spec>(is_cold: bool) -> u64 {
    if SPEC::enabled(BERLIN) {
    if is_cold {
    COLD_ACCOUNT_ACCESS_COST
    } else {
    WARM_STORAGE_READ_COST
    }
    } else if SPEC::enabled(TANGERINE) {
    // EIP-150: Gas cost changes for IO-heavy operations
    700
    } else {
    40
    }
    }
@mattsse
Copy link
Collaborator

mattsse commented Mar 24, 2024

one note here is that I think we want a more flexible solution for the SpecId enum that would allow us to get rid of the duplicated op and ethereum SpecId enum.
because we want to abstract away all the OP cfgs

@rakita
Copy link
Member

rakita commented Mar 25, 2024

one note here is that I think we want a more flexible solution for the SpecId enum that would allow us to get rid of the duplicated op and ethereum SpecId enum. because we want to abstract away all the OP cfgs

@mattsse this is a different Issue, seems not related to this one. #918

Usage of the SPEC generic is unnecessary in gas calculation code as the SpecId is already known at compile-time in the instructions and these functions are marked with #[inline], so they would get optimized the same way.

By turning the SPEC generic into a SpecId parameter:

  1. most gas calculation functions can be made const, allowing usage in compile-time gas tables and outside of SPEC-generic code
  2. helps a bit with compile times by reducing monomorphization bloat

SPEC::enabled(X) becomes SpecId::enabled(spec, X).

This likely applies to more places in revm-interpreter, but I haven't looked too deeply into it, and I'm confident that gas calculation is a good starting place.

In general supportive, it looks like a refactor without a lot of gains, but it can potentially be useful to const fn outside of Revm.

But would use spec_Id.is_enabled_in(BERLIN) as SpecId::enabled(BERLIN, X) can be a footgun with Spec order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers refactor Refactor of the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants