Skip to content
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.

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

(LedgerStore) LedgerColumn::multi_get() #26354

Merged
merged 1 commit into from
Sep 12, 2022
Merged

(LedgerStore) LedgerColumn::multi_get() #26354

merged 1 commit into from
Sep 12, 2022

Conversation

yhchiang-sol
Copy link
Contributor

@yhchiang-sol yhchiang-sol commented Jul 1, 2022

Problem

Blockstore operations such as get_slots_since() issues multiple rocksdb::get()
at once which is not optimal for performance.

Summary of Changes

This PR adds LedgerColumn::multi_get() based on rocksdb::batched_multi_get(),
the optimized version of multi_get() where get requests are processed in batch
to minimize read I/O.

@yhchiang-sol
Copy link
Contributor Author

yhchiang-sol commented Jul 1, 2022

The multi_get() API will be used to optimize get_slots_since() mentioned in #24878

@yhchiang-sol yhchiang-sol marked this pull request as draft July 6, 2022 15:34
@yhchiang-sol
Copy link
Contributor Author

Converting this PR back to a draft as we need a published version of rust-rocksdb release.

@yhchiang-sol yhchiang-sol requested a review from steviez July 6, 2022 15:35
@stale
Copy link

stale bot commented Aug 13, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 13, 2022
@yhchiang-sol yhchiang-sol removed the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 27, 2022
@yhchiang-sol yhchiang-sol marked this pull request as ready for review August 28, 2022 05:47
ledger/src/blockstore.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_db.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_db.rs Outdated Show resolved Hide resolved
steviez
steviez previously approved these changes Sep 9, 2022
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Just the clippy thing; otherwise, LGTM!

Out of curiosity, any idea what kind of improvements this yields vs. just calling get() repeatedly ?

@@ -1289,6 +1307,41 @@ impl<C> LedgerColumn<C>
where
C: TypedColumn + ColumnName,
{
#[allow(clippy::needless_range_loop)]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to drop this now right?

@yhchiang-sol
Copy link
Contributor Author

Out of curiosity, any idea what kind of improvements this yields vs. just calling get() repeatedly ?

So the batched version of multi_get() will try to group multiple get() requests into multiple batches where get() requests in the same batch will hit the same block. As a result, each batch of get() requests can be answered using one single disk read instead of multiple ones.

@mergify mergify bot dismissed steviez’s stale review September 12, 2022 05:05

Pull request has been modified.

@yhchiang-sol yhchiang-sol merged commit ba3d9cd into solana-labs:master Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants