Skip to content

Remove shrink_to_fit from InMemAccountsIndex eviction #8823

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

Merged
merged 2 commits into from
Nov 6, 2025
Merged

Remove shrink_to_fit from InMemAccountsIndex eviction #8823

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d0cab6
shrink accounts index after eviction
HaoranYi Nov 1, 2025
648258a
Remove shrink_to_fit from InMemAccountsIndex eviction
HaoranYi Nov 5, 2025
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
3 changes: 0 additions & 3 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 @@ -1247,9 +1247,6 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> InMemAccountsIndex<T,
occupied.remove();
}
}
if map.is_empty() {
map.shrink_to_fit();
}
Comment on lines -1250 to -1252
Copy link

@brooksprumo brooksprumo Nov 5, 2025

Choose a reason for hiding this comment

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

Can you look up why this was added in the first place? I don't want to remove this until I understand why it was added.

Copy link
Author

@HaoranYi HaoranYi Nov 5, 2025

Choose a reason for hiding this comment

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

solana-labs#23795

This was added in the above PR when we switched to chunked eviction, but it doesn’t seem necessary there either.

Copy link

@brooksprumo brooksprumo Nov 5, 2025

Choose a reason for hiding this comment

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

In that PR (solana-labs#23795), the if-empty-then-shrink block is in both sides of the diff, so I don't think that's the PR that introduced the shrink-to-fit originally.

Copy link
Author

@HaoranYi HaoranYi Nov 5, 2025

Choose a reason for hiding this comment

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

Ah, that makes sense — this was added back when we introduced the disk index in 2021.

At that time, we built the index fully in memory first and then flushed it to disk, so the shrink_to_fit() was needed to trim the in-memory map after the flush.

In today’s code, though, we flush directly to disk during startup, and the in-memory map should always be empty. So the shrink is no longer necessary.

git show 181c0092d623f9319af150491256efbc5f8f7987

commit 181c0092d623f9319af150491256efbc5f8f7987
Author: Jeff Washington (jwash) <[email protected]>
Date:   Wed Dec 8 16:52:22 2021 -0600

    AcctIdx: resize in-mem after startup for disk index (#21676)

Copy link

@brooksprumo brooksprumo Nov 5, 2025

Choose a reason for hiding this comment

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

Nice, and that matches up with why this code existed in the first place. Thanks for finding the original PR again.

Can you run this PR on a validator, with disk index enabled, and share the index memory usage + validator memory usage, please?

Copy link
Author

@HaoranYi HaoranYi Nov 6, 2025

Choose a reason for hiding this comment

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

image

Around 10:40 is the restart of the validator with this PR + disk-index enabled.
Before it was running with threshold index flush (50M).
With disk-index, it used less memory and has less hashmap count as usual.

Therefore, remove this shrink has no impact for disk-index.

let capacity_post = map.capacity();
drop(map);
stats.update_in_mem_capacity(capacity_pre, capacity_post);
Expand Down
Loading