Adds get_with_and_then() to AccountsIndex #35307
Conversation
brooksprumo
force-pushed
the
self-ref/2/get_with_and_then
branch
from
7e63ed4 to
293a926
Compare
accounts-db/src/accounts_index.rs
Outdated
| /// Gets the index's entry for `pubkey`, with `ancestors` and `max_root`, | ||
| /// and applies `callback` to it | ||
| pub(crate) fn get_with_and_then<R>( | ||
| &self, | ||
| pubkey: &Pubkey, | ||
| ancestors: Option<&Ancestors>, | ||
| max_root: Option<Slot>, | ||
| should_add_to_in_mem_cache: bool, | ||
| mut callback: impl FnMut((Slot, T)) -> R, | ||
| ) -> Option<R> { | ||
| self.get_and_then(pubkey, |entry| { | ||
| let callback_result = entry.and_then(|entry| { | ||
| let slot_list = entry.slot_list.read().unwrap(); | ||
| self.latest_slot(ancestors, &slot_list, max_root) | ||
| .map(|found_index| callback(slot_list[found_index])) | ||
| }); | ||
| (should_add_to_in_mem_cache, callback_result) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Here's the main new function. In subsequent PRs more calls to get() will be replaced with calls to get_with_and_then().
accounts-db/src/accounts_index.rs
Outdated
| @@ -1154,6 +1174,17 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> AccountsIndex<T, U> { | |||
| self.get_and_then(pubkey, |entry| (false, entry.is_some())) | |||
| } | |||
|
|
|||
| /// Is `pubkey`, with `ancestors` and `max_root`, in the index? | |||
| pub fn contains_with( | |||
There was a problem hiding this comment.
Yeah, I can do that. For the most part, all of AccountsIndex really should not be public to the world, only the accounts db crate, right?
Another way to handle this is to remove the pub when importing the module in accounts-db/src/lib.rs.
(Mostly trying to avoid (1) unnecessary CI runs and (2) creating inconsistency within the accounts index code.)
There was a problem hiding this comment.
Done in 97c3313.
There was a problem hiding this comment.
My thought process is that contains() and get() et al really are the public APIs for AccountsIndex. So pub makes sense in my head.
Simultaneously we do not want others to call into AccountsIndex.
So only AccountsDb would have visibility to AccountsIndex, and all others would not. Thus only AccountsDb could call AccountsIndex functions.
There was a problem hiding this comment.
Ah bummer... Because this PR doesn't use get_with_and_then() nor contains_with() outside of tests, clippy is throwing errors saying these functions are dead code 🫠
Do you have a preference for resolution? I could mark these functions are dead code in this PR, and remove the annotations in future PRs. Or I could change the vis to pub instead of pub(crate). Or something else?
There was a problem hiding this comment.
Done. Marked as dead code in 02b209e.
I'm ok with marked as dead code. Isn't there a cfg[test] or something? Not a huge deal. Just trying to get it right with each commit.
There was a problem hiding this comment.
Yeah, I could decorate the functions with #[cfg(test)] too. Would you prefer I change to use #[cfg(test)]?
Just trying to get it right with each commit.
Yah, I prefer that way too. IMO I still think pub is the right visibility here for these functions 😸
There was a problem hiding this comment.
#[cfg(test)]
yes, that's what I'd prefer.
There was a problem hiding this comment.
I still think
pubis the right visibility
I'm inclined to say make as little pub as possible. If this can be not pub, don't make it pub. Then we can get rid of it or change it later without any concern.
There was a problem hiding this comment.
Decorated with #[cfg(test)] in 24acb5f.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #35307 +/- ##
========================================
Coverage 81.7% 81.7%
========================================
Files 834 834
Lines 224197 224310 +113
========================================
+ Hits 183287 183389 +102
- Misses 40910 40921 +11 |
Problem
See #34786 for background.
We want to limit the use of
ReadAccountMapEntry, from AccountsIndex, everywhere. Ultimately removing it once there are no more uses.There are many uses in tests of
get()just to check if a pubkey is in the index, or a specific value for the index entry's account info at a specific slot. These uses do not need to useget().Summary of Changes
Add a family of functions to AccountsIndex for getting an entry—with given ancestors and max root—in the Index without creating a ReadAccountMapEntry.
Subsequent PRs will iteratively replace more and more calls to get() with get_with_and_then() until they are all gone.