chore: refactor L1BlockInfo::tx_estimated_size_fjord
#1856
Conversation
|
@hai-rise could you please also submit these improvements to the https://github.com/bluealloy/revm/tree/release/v50 otherwise these improvements won't make it into the release |
| @@ -161,12 +171,14 @@ impl L1BlockInfo { | |||
| // This value is computed based on the following formula: | |||
| // max(minTransactionSize, intercept + fastlzCoef*fastlzSize) | |||
| fn tx_estimated_size_fjord(&self, input: &[u8]) -> U256 { | |||
There was a problem hiding this comment.
should we change this to u64?
looks like we use this in other places where we can also operate on u64
There was a problem hiding this comment.
The call in calculate_tx_l1_cost_fjord multiples with a large l1_fee_scaled so U256 is needed.
The call in data_gas looks pretty safe, but is not in any hot path as data_gas is only called in calculate_tx_l1_cost_bedrock and calculate_tx_l1_cost_ecotone that don't pass in a FJORD-enabled spec.
I'd say it's not worth it as it forces callers to mentally track u64::MAX to not wrap in a U256 anyway. It's easier within tx_estimated_size_fjord as we know even a u32::MAX returned by flz_compress_len wouldn't overflow u64.
|
Thanks again for measuring this |
Found
tx_estimated_size_fjordin our block-building pipeline's hot path so had a look.Apparently, it's taking a non-trivial 9% of the cake when
State::committakes 10% andEvm::transactitself only takes 38%.This PR adds some info and constants around
tx_estimated_size_fjord, and micro-optimizes it by removing redundantU256conversions. 91% oftx_estimated_size_fjordis still spent onflz_compress_lenso we may need to optimize the current Rust port, or go with an Assembly binding.