feat: gas profile by parameter #8148
Conversation
core/primitives-core/src/profile.rs
Outdated
| for (cost, gas) in self.actions_profile.iter_mut() { | ||
| *gas = gas.saturating_add(other.actions_profile[cost]); | ||
| } |
There was a problem hiding this comment.
Consider
| for (cost, gas) in self.actions_profile.iter_mut() { | |
| *gas = gas.saturating_add(other.actions_profile[cost]); | |
| } | |
| for ((_, gas), (_, other_gas)) in self.actions_profile.iter_mut().zip(other.actions_profile.iter()) { | |
| *gas = gas.saturating_add(*other_gas); | |
| } |
There was a problem hiding this comment.
Hm, feels more complicated to achieve the same thing? Anything wrong with the original code?
There was a problem hiding this comment.
Indexing of the other.actions_profile[cost] is a panicky operation, and it needs to bound check every time too (as the indices are going upwards). The zip approach, avoids both of these problems by a) necessarily picking a handling mechanism (truncation) when the lengths mismatch, and b) being an iterator, thus not needing bound checks.
There was a problem hiding this comment.
Ah, these are convincing arguments. Updated!
|
Hey @nagisa thanks for reviewing! Sorry, I only created this draft PR to run tests on CI, it builds on the changes in #8115 which have been hanging in review for 2 weeks. This probably made it a bit harder to review than necessary. Now that the other PR is merged, I will rebase this PR, address your comments and then mark it as ready for review afterwards. |
- use ProfileDataV2 in new profiles - removed all methods from ProfileDataV1 used to create new profiles - added borsh support to ProfileDataV2 - test for borsh
- name profile data versions to be consistent with meta data versions - move deprecated profile to profile_v2.rs - use enum discriminants instead of manual index mapping - add changelog entry for new gas profiles - update tests
jakmeier
force-pushed
the
new-gas-profiles
branch
from
ee8eaa3 to
9d29a05
Compare
|
Should be almost ready now but I want to add some tests for the RPC view output. I'll first need to work on more urgent things though... |
- remove zero gas entries - upper case for parameter names
also don't run snapshot tests on nightly, differences are expected there
|
@nagisa do you think you will have time to look at this and are the right person to review it? I can also ask someone else but since you have already looked at an early version once (back in early December) you are the natural candidate. |
|
Aha, thanks for the reminder, I’m looking at it now. |
core/primitives/src/views.rs
Outdated
| // Can't `sort_by_key` here because lifetime inference in | ||
| // closures is limited. | ||
| costs.sort_by(|lhs, rhs| { | ||
| lhs.cost_category.cmp(&rhs.cost_category).then(lhs.cost.cmp(&rhs.cost)) |
There was a problem hiding this comment.
I suspect clippy would complain about the then here, possibly saying something about it being executed regardless of whether the first check passed or not. I suspect the compiler optimizes this out well either way, though…
There was a problem hiding this comment.
Ah right, then_with is more appropriate.
This resolves #8033, completing the gas profile by parameter effort.
The main achievement here is that we can now look at gas profiles and
know exactly for which parameter it has been spent.
Code wise, the old
enum Costused in profiles only is finally gone.The tricky parts in this PR are to make sure that old profiles stored on DB
are still understood and presented correctly in RPC calls.