Skip to content

Reimplement index scan iter to not use hold_range_in_memory #6917

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.

Sign up for GitHub

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

Closed
HaoranYi wants to merge 1 commit into from
Closed

Reimplement index scan iter to not use hold_range_in_memory #6917

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions accounts-db/src/accounts_index/in_mem_accounts_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,22 +275,41 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> InMemAccountsIndex<T,

let m = Measure::start("items");

// For simplicity, we check the range for every pubkey in the map. This
// can be further optimized for case, such as the range contains lowest
// and highest pubkey for this bin. In such case, we can return all
// items in the map without range check on item's pubkey. Since the
// check is cheap when compared with the cost of reading from disk, we
// are not optimizing it for now.
self.hold_range_in_memory(range, true);
let result = self
.map_internal
.read()
.unwrap()
// We need to hold the flushing_active lock first before gathering the items from the index.
#[allow(unused_assignments)]
let mut flush_guard = None;
loop {
flush_guard = FlushGuard::lock(&self.flushing_active);
if flush_guard.is_some() {
break;
}
}
Comment on lines +278 to +286
Copy link
Copy Markdown

@brooksprumo brooksprumo Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the FlushGuard instead of the EvictionsGuard?

(forgive me, I've forgotten a lot of the details here. I see in `hold_range_in_memory() we grab the EvictionsGuard.)

Copy link
Copy Markdown
Author

@HaoranYi HaoranYi Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EvictionsGuard will be removed too, see #6920. It is only relevant when we are holding range in the index.
Here we just need to lock flush_guard, which prevent evict_from_cache.
flush_internal calls evict_from_cache.

Copy link
Copy Markdown
Author

@HaoranYi HaoranYi Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To give a broader context, please see #6920, which is the PR does the final removal.

Copy link
Copy Markdown

@brooksprumo brooksprumo Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going back over the code, I think we use the FlushGuard to indicate that we are flushing. We aren't flushing here, but I guess it is being used to prevent other flushing from happening. However, the existing flush() functions do not spin on the FlushGuard. I haven't thought through all the implications here.

Also, just below we do self.map_internal.write().unwrap() and grab the write lock on the in-mem entries for this bin. That seems sufficient enough to prevent flushing, since in flush_scan() it grabs the read lock and evict_from_cache() grabs the write lock.

I think that means we can remove the FlushGuard here too?

(And probably change the write lock below to a read lock?)


// Collect items from the in-memory map that are within the range.
// We hold a write lock to the in-memory map to ensure that no other thread is modifying it while we are reading.
let map = self.map_internal.write().unwrap();
let mut index_items: HashMap<_, _> = map
.iter()
.filter(|&(k, _v)| range.contains(k))
.map(|(k, v)| (*k, Arc::clone(v)))
.collect();
self.hold_range_in_memory(range, false);

// Collect items from the disk if they are not in-memory.
if let Some(disk) = self.bucket.as_ref() {
let items = disk.items_in_range(&Some(range));
for item in items {
if !map.contains_key(&item.pubkey) {
index_items.insert(
item.pubkey,
self.disk_to_cache_entry(item.slot_list, item.ref_count),
);
}
}
}
drop(map);
drop(flush_guard);

let result = index_items.into_iter().collect();
Self::update_stat(&self.stats().items, 1);
Self::update_time_stat(&self.stats().items_us, m);
result
Expand Down
Loading