-
Notifications
You must be signed in to change notification settings - Fork 201
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
fix: borrow stakes delegation during snapshot serialization #2455
Conversation
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
Adding v2.0 backport to reduce memory allocation when serializing epoch stakes entries as well. |
runtime/src/stakes.rs
Outdated
@@ -544,6 +547,23 @@ impl From<Stakes<StakeAccount>> for Stakes<Delegation> { | |||
} | |||
} | |||
|
|||
impl<'a> From<&'a Stakes<StakeAccount>> for Stakes<&'a Delegation> { | |||
fn from(stakes: &'a Stakes<StakeAccount>) -> Self { |
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.
it looks like this is only used by serialize(), is that right? if so, I think we could
avoid this big allocation entirely and stream-serialize the input
&Stakes<StakeAccount>
without creating this temporary Stakes<&Delegation>
directly in the caller?
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.
Yeah good call, let me know what you think of 36b5a16
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.
New commit here with generics and serialize-with removed: e004718
@ryoqun I might need some help with this abi example error which I believe is coming from using generics:
I tried removing the generics too (can share the code if you want) but getting this error still:
|
Nvm @ryoqun I was able to make it work by not using |
36b5a16
to
e004718
Compare
@@ -227,6 +227,13 @@ impl StakeStateV2 { | |||
} | |||
} | |||
|
|||
pub fn delegation_ref(&self) -> Option<&Delegation> { |
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 for the future: I don't think it's worth having both delegation() and
delegation_ref()
It looks like we could have delegation return a ref, and then callers .clone()
if they need to?
Since we're backporting this it's probably better to add delegation_ref() for
now to minimize the diff, but something to keep in mind for the future.
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.
Totally agree but didnt want to break the sdk api
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.
looks great!
(cherry picked from commit c99095d) # Conflicts: # runtime/src/bank/serde_snapshot.rs # runtime/src/stakes.rs
Btw I plan to make similar improvements to the versioned epoch stakes serialization code |
(cherry picked from commit c99095d) # Conflicts: # runtime/src/bank/serde_snapshot.rs # runtime/src/stakes.rs
(cherry picked from commit c99095d) # Conflicts: # runtime/src/bank/serde_snapshot.rs # runtime/src/stakes.rs
The Firedancer team maintains a line-for-line reimplementation of the |
Problem
When serializing snapshots, we do a full deep clone of the stake delegations when serializing
Stakes<Delegation>
. As of v2.1 this serialization flow is limited to serializing the stakes cache since epoch stakes are serialized with versioned epoch stakes as of #2282.Summary of Changes
Build a shallow copy of stake delegations by borrowing the
delegation
field from the stake accounts to avoid allocating so much when serializingStakes<Delegation>
during snapshot creation.Fixes #