Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Replaces ReadAccountMapEntry in snapshot minimizer #35237

Merged

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Feb 19, 2024

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.

When creating a minimized snapshot we query the Accounts Index to find which pubkeys are used, and in which slots. The impl currently uses ReadAccountMapEntry, but can be rewritten to avoid it.

Summary of Changes

Replaces get_account_read_entry() in snapshot minimizer.

@brooksprumo brooksprumo added the work in progress This isn't quite right yet label Feb 19, 2024
@brooksprumo brooksprumo self-assigned this Feb 19, 2024
@brooksprumo brooksprumo force-pushed the self-ref/2/snapshot-minimizer branch from 1f5a75e to 976d326 Compare February 19, 2024 15:28
@brooksprumo brooksprumo removed the work in progress This isn't quite right yet label Feb 19, 2024
@brooksprumo brooksprumo changed the title Replaces get_account_read_entry() in snapshot minimizer Replaces ReadAccountMapEntry in snapshot minimizer Feb 19, 2024
@brooksprumo brooksprumo marked this pull request as ready for review February 19, 2024 16:13
@brooksprumo
Copy link
Contributor Author

@apfitzge Requesting your review because I'm touching the snapshot minimizer code
@HaoranYi @jeffwashington Requesting your review due to the Accounts Index changes/use

Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Changes look good to me; seems there's no functional difference in the behavior of minimizer, but would remove some heap allocations by not moving the arced-entry into a Box

Comment on lines +265 to +266
.read()
.unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: previously the ReadAccountMapEntry held this lock internally, so no added contention here.

@apfitzge
Copy link
Contributor

Changes look good to me; seems there's no functional difference in the behavior of minimizer, but would remove some heap allocations by not moving the arced-entry into a Box

should also be saving on the removed Arc::clone, nice!

Copy link
Contributor

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo merged commit 915faab into solana-labs:master Feb 20, 2024
36 checks passed
@brooksprumo brooksprumo deleted the self-ref/2/snapshot-minimizer branch February 20, 2024 18:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants