Make flushing of unrooted slots explicit#9960
Conversation
| let mut flush_stats = FlushStats::default(); | ||
| old_slots.into_iter().for_each(|old_slot| { | ||
| // Don't flush slots that are known to be unrooted | ||
| // Slots <= max_flushed_root can never become roots later (any unrooted slot that |
There was a problem hiding this comment.
This comment was incredibly confusing to me (and has been everytime i've looked at this function). Updated.
|
|
||
| pub fn flush_accounts_cache_slot_for_tests(&self, slot: Slot) { | ||
| self.flush_slot_cache(slot); | ||
| assert!( |
There was a problem hiding this comment.
Originally I retained flush_slot_cache and added the root assert, but it was only being used in flush_accounts_cache_slot_for_tests which lead to unused function errors. Decided this was the cleanest way to fix it.
There was a problem hiding this comment.
Might be useful to look at each commit individually.
| fn flush_slot_cache(&self, slot: Slot) -> Option<FlushStats> { | ||
| /// Flushes an unrooted slot from the write cache to storage to free up memory | ||
| #[cfg_attr(feature = "dev-context-only-utils", qualifiers(pub))] | ||
| fn flush_unrooted_cache_slot(&self, slot: Slot) -> Option<FlushStats> { |
There was a problem hiding this comment.
This diff looks a little weird. There are two changes
- flush_unrooted_cache_slot
- Removing flush_slot_cache
| .contains(&slot), | ||
| "slot: {slot}" | ||
| ); | ||
| self.flush_slot_cache_with_clean(slot, None::<&mut fn(&_) -> bool>, None); |
There was a problem hiding this comment.
This doesn't clean because should_clean is None. Cleaning is the desired behaviour (since it is the default behaviour), but I will handle that in another PR.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9960 +/- ##
=========================================
- Coverage 82.5% 82.5% -0.1%
=========================================
Files 844 844
Lines 316472 316485 +13
=========================================
+ Hits 261309 261314 +5
- Misses 55163 55171 +8 🚀 New features to boost your workflow:
|
roryharr
force-pushed
the
explicit_flush_unrooted_slot
branch
from
9a5629d to
f52212c
Compare
brooksprumo
left a comment
brooksprumo
left a comment
There was a problem hiding this comment.
These items are still open imo:
- Can we put the assert in here (vs the test fn) to ensure
slotis unrooted?
Removing changes to tests that are not using new function
3 participants
Problem
Flushing unrooted slots should only be done when the accounts cache exceeds the expected size. Make flushing of unrooted slots explicit in general to avoid unintentionally flushing unrooted slots
Summary of Changes
Note:
This is currently relevant as some of the expectations around clean will need to change as the handling of zero lamport accounts only accessed in a single slot (aka ephemeral accounts) will change.
Fixes #