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

(Ledger Store Benchmark) Enable FIFO Compaction in the benchmark #22162

Merged
merged 1 commit into from
Feb 12, 2022
Merged

(Ledger Store Benchmark) Enable FIFO Compaction in the benchmark #22162

merged 1 commit into from
Feb 12, 2022

Conversation

yhchiang-sol
Copy link
Contributor

@yhchiang-sol yhchiang-sol commented Dec 29, 2021

Summary of Changes

This PR enables FIFO Compaction in the test_ledger_cleanup_compaction benchmark.

Note that this PR is based on top of two stack PRs: #22108 and #22140.

To enable FIFO compaction, the following arguments should be set:

FIFO_COMPACTION=true
CLEANUP_SERVICE=false
SHRED_DATA_CF_SIZE=10000000000 // this controls when to delete the oldest SST file.

Example command to use FIFO compaction in the benchmark:

BENCHMARK_SLOTS=10000000 NUM_WRITERS=9 BATCH_SIZE=1 SHREDS_PER_SLOT=25 PRE_GENERATE_DATA=false CLEANUP_BLOCKSTORE=false CLEANUP_SERVICE=false FIFO_COMPACTION=true SHRED_DATA_CF_SIZE=10000000000 cargo test --release tests::test_ledger_cleanup_compaction -- --exact --nocapture

Observed the FIFO compaction working properly by checking the output and the RocksDB LOG file.

@yhchiang-sol
Copy link
Contributor Author

yhchiang-sol commented Dec 29, 2021

Here're some example logs showing the effectiveness of the FIFO compaction. I will run more benchmarks comparing its results with other settings:

[data_shred] Level-0 commit table #79 started. 
[data_shred] Level-0 commit table #79: memtable #1 done // created the first SST file.
[data_shred] Level summary: files[1] max score 0.05
[data_shred] Level-0 commit table #81 started
[data_shred] Level-0 commit table #81: memtable #1 done // created the 2nd SST file.
[data_shred] Level summary: files[2] max score 0.10
...
...
[data_shred] Level-0 commit table #128 started
[data_shred] Level-0 commit table #128: memtable #1 done
[data_shred] Level summary: files[20] max score 1.03 // reached the max column family size
[data_shred] FIFO compaction: picking file 79 with size 490MB for deletion. // correctly deleting the oldest SST file
[data_shred] Deleted 1 files
...
[data_shred] Level-0 commit table #130 started
[data_shred] Level-0 commit table #130: memtable #1 done
[data_shred] Level summary: files[20] max score 1.03
[data_shred] FIFO compaction: picking file 81 with size 490MB for deletion // correctly deleting the oldest SST file
[data_shred] Deleted 1 files

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #22162 (4b5799c) into master (69b4791) will decrease coverage by 0.0%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #22162     +/-   ##
=========================================
- Coverage    81.3%    81.2%   -0.1%     
=========================================
  Files         567      567             
  Lines      154287   154287             
=========================================
- Hits       125456   125433     -23     
- Misses      28831    28854     +23     

Comment on lines 320 to 354
match options.shred_storage_type {
ShredStorageType::RocksLevel => {
new_cf_descriptor::<ShredData>(&access_type, &oldest_slot)
}
ShredStorageType::RocksFifo => {
if options.shred_data_cf_size > FIFO_WRITE_BUFFER_SIZE {
new_cf_descriptor_fifo::<ShredData>(&options.shred_data_cf_size)
} else {
warn!(
"shred_data_cf_size is must be greater than {} for RocksFifo.",
FIFO_WRITE_BUFFER_SIZE
);
warn!("Fall back to ShredStorageType::RocksLevel for cf::ShredData.");
new_cf_descriptor::<ShredData>(&access_type, &oldest_slot)
}
}
},
match options.shred_storage_type {
ShredStorageType::RocksLevel => {
new_cf_descriptor::<ShredCode>(&access_type, &oldest_slot)
}
ShredStorageType::RocksFifo => {
if options.shred_code_cf_size > FIFO_WRITE_BUFFER_SIZE {
new_cf_descriptor_fifo::<ShredCode>(&options.shred_code_cf_size)
} else {
warn!(
"shred_code_cf_size is must be greater than {} for RocksFifo.",
FIFO_WRITE_BUFFER_SIZE
);
warn!("Fall back to ShredStorageType::RocksLevel for cf::ShredCode.");
new_cf_descriptor::<ShredCode>(&access_type, &oldest_slot)
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract this into a common function that takes a generic T to distinguish between ShredCode and ShredData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code was on its stacked PR which is already landed.

I've created #23103 for this.

Comment on lines 1024 to 1033
pub shred_data_cf_size: u64,
// The maximum storage size for storing coding shreds in column family
// [`cf::CodeShred`]. Typically, coding shreds contribute around 20% of the
// ledger store storage size if the RPC service is enabled, or 40% if RPC
// service is not enabled.
//
// Currently, this setting is only used when shred_storage_type is set to
// [`ShredStorageType::RocksFifo`].
pub shred_code_cf_size: u64,
Copy link
Contributor

@carllin carllin Feb 10, 2022

Choose a reason for hiding this comment

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

Since these two fields are only necessary when FIFO compaction is enabled, how about a separate struct FifoCompactionOptions, and we have an optional field here: fifo_compaction_options: Option<FifoCompactionOptions>. Then the default is None for leveled compaction, which communicates the dependency between these fields and fifo compaction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two are from its parent PR (which is already merged.). I will incorporate the comments in a separate PR.

@yhchiang-sol yhchiang-sol merged commit 8244467 into solana-labs:master Feb 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