Clones skipped rewrites instead of taking #34311
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #34311 +/- ##
=======================================
Coverage 81.9% 81.9%
=======================================
Files 819 819
Lines 220084 220119 +35
=======================================
+ Hits 180347 180409 +62
+ Misses 39737 39710 -27 |
| @@ -6909,7 +6909,7 @@ impl Bank { | |||
| .calculate_accounts_delta_hash_internal( | |||
| slot, | |||
| ignore, | |||
| std::mem::take(&mut self.skipped_rewrites.lock().unwrap()), | |||
| self.skipped_rewrites.lock().unwrap().clone(), | |||
There was a problem hiding this comment.
another option, with no runtime cost, is to pass &RwLock<HashMap...>>.
There was a problem hiding this comment.
we could also use something like the OnceLock
There was a problem hiding this comment.
Yep! I originally didn't want to modify calculate_accounts_delta_hash_internal(), which takes the skipped rewrites by value and mutates it.
We can borrow the skipped rewrites and pass a reference into calculate_accounts_delta_hash_internal(). Inside though, we need to only add the rewrites that were not already in the vec of modified accounts in this slot. Currently that iterates the vec of modified accounts and removes any matches in the skipped rewrites.
If we do not modify the skipped rewrites, then I think we'll need to selectively add the skipped rewrites to the vec by searching the vec for each pubkey that's in the skipped rewrites. This feel slow, but I'm not sure if it's faster/slower than the clone.
Wdyt? Or would you like me to get runtime numbers for both versions?
There was a problem hiding this comment.
ugh. I forgot we modify it. clone is fine.
Problem
While debugging a test in #34280, I discovered the issue was due to skipped rewrites getting taken when doing the accounts delta hash calculation. Usually this isn't a problem. But, when we call
bank_to_xxx_snapshot_archive(), we callBank::rehash(), which does another accounts delta hash calculation. Except this second time there are no more skipped rewrites! This means the bank hash in the snapshot is always wrong (when skipping rewrites but also including them in the bank hash).In #34280 we are solving that by rebuilding the skipped rewrites. Maybe that's not the best solution? We can also fix the problem very simply by cloning the skipped rewrites instead of using
take().Summary of Changes
Clone the skipped rewrites instead of taking them.
Note that the new test will fail without cloning the skipped rewrites.