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

chunk all ancient append vecs #33909

Merged
merged 15 commits into from
Nov 7, 2023
Merged

Conversation

jeffwashington
Copy link
Contributor

@jeffwashington jeffwashington commented Oct 27, 2023

Problem

When we pack ancient vecs, we end up with non-ancient append vecs interleaved with ancient append vecs.
This ends up causing hash calculation to take much longer. Hash Calculation is very sensitive to the # of chunks.
The current chunking algorithm assumes the model of appending ancient append vecs.

Summary of Changes

Just create ranges instead.
image
The blue line that drops here is restarting with the removal of the ancient splitter algorithm (this pr).

Here's the overall hash calc time with removing this code:
image

Fixes #

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #33909 (6435e5f) into master (da130b8) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is 80.0%.

@@           Coverage Diff           @@
##           master   #33909   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         811      811           
  Lines      219335   219356   +21     
=======================================
+ Hits       179710   179758   +48     
+ Misses      39625    39598   -27     

@HaoranYi HaoranYi self-assigned this Oct 28, 2023
@HaoranYi HaoranYi marked this pull request as ready for review November 6, 2023 15:31
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Looking good. And to confirm, the perf numbers are still the same as what's in the original/current PR description?

accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
accounts-db/src/accounts_db.rs Show resolved Hide resolved
accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
@HaoranYi
Copy link
Contributor

HaoranYi commented Nov 6, 2023

Looking good. And to confirm, the perf numbers are still the same as what's in the original/current PR description?

Yeah.
image

brooksprumo
brooksprumo previously approved these changes Nov 6, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm!

@@ -250,6 +253,21 @@ impl HashStats {
("collect_snapshots_us", self.collect_snapshots_us, i64),
("num_snapshot_storage", self.num_snapshot_storage, i64),
("scan_chunks", self.scan_chunks, i64),
(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe these stats already do what you are after?

(
"loaded_from_cache",
self.loaded_from_cache.load(Ordering::Relaxed),
i64
),
(
"saved_to_cache",
self.saved_to_cache.load(Ordering::Relaxed),
i64
),
(
"entries_loaded_from_cache",
self.entries_loaded_from_cache.load(Ordering::Relaxed),
i64
),

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. These stats are what I want. I removed the redundant stats that I added.

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm

@jeffwashington
Copy link
Contributor Author

lgtm. thanks, @HaoranYi

@HaoranYi HaoranYi merged commit b8115b4 into solana-labs:master Nov 7, 2023
32 checks passed
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.

3 participants