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

Update "limit-ledger-size" to use DeleteRange for much faster deletes #7515

Merged
merged 12 commits into from Dec 18, 2019

Conversation

@sagar-solana
Copy link
Contributor

sagar-solana commented Dec 16, 2019

Problem

--limit-ledger-size calls blocktree's purge_slots api which scans through index ranges on each CF and calls delete on each one. This has explicitly been discouraged in the RocksDb Docs.
We should instead use DeleteRange on the entire range of indexes.

See: https://github.com/facebook/rocksdb/wiki/DeleteRange

Summary of Changes

  • Updated Blocktree purge_slots to use DeleteRange on a CF instead of deleting each index manually.
  • Delete in batches of 256 slots instead of triggering purge on individual slots, it's more efficient to do so.
  • Reduced number of epochs to keep around to 2. (4 weeks in prod runs)

Delete is much faster.

To delete 5000 slots with 1000 entries per slot...

Old Purge took 28.549164ms
New Purge took 258.309µs

#Fixes #6521

@sagar-solana sagar-solana requested review from carllin and pgarg66 Dec 16, 2019
@mergify mergify bot dismissed pgarg66’s stale review Dec 16, 2019

Pull request has been modified.

@sagar-solana sagar-solana added the CI label Dec 16, 2019
@solana-grimes solana-grimes removed the CI label Dec 16, 2019
@sagar-solana sagar-solana force-pushed the sagar-solana:fix_limit_ledger_stalls branch from b16ce1f to 238812c Dec 16, 2019
@danpaul000 danpaul000 added the CI label Dec 16, 2019
@solana-grimes solana-grimes removed the CI label Dec 16, 2019
let from_index = C::as_index(from);
let to_index = C::as_index(to);
let result = batch.delete_range_cf::<C>(cf, from_index, to_index);
let max_slot = self

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 17, 2019

Author Contributor

@pgarg66 I added logic to detect if we've reached the end of the column to allow the caller to stop looping if purge_slots is given a None on our upper bound. I forgot to handle this and it was causing tests to loop :D

.db
.delete_range_cf::<cf::SlotMeta>(&mut write_batch, from_slot, to_slot)
.unwrap_or_else(|_| false)
& self

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 17, 2019

Author Contributor

Also found a major bug.

I forgot that && short circuits the evaluation and so had to switch to & to make sure all columns were deleted properly.

This is probably why limit-ledger-size was not doing anything. 🤦‍♂

This comment has been minimized.

Copy link
@mvines

This comment has been minimized.

Copy link
@mvines

mvines Dec 17, 2019

Member

Think you can easily backport part of this to 0.21 today? We've had multiple slp1 validators run out of disk space already

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 17, 2019

Author Contributor

Early test results were not promising. There was no difference. I think with DeleteRange we need compaction to actually delete the slots. Otherwise it just marks a tombstone and calls it a day. I'll investigate today.

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 17, 2019

Author Contributor

Yeah i need to test if it fixes the older delete logic better than the DeleteRange pathway. I'll work on it today.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 17, 2019

Codecov Report

Merging #7515 into master will increase coverage by <.1%.
The diff coverage is 61.1%.

@@           Coverage Diff            @@
##           master   #7515     +/-   ##
========================================
+ Coverage    65.6%   65.6%   +<.1%     
========================================
  Files         245     245             
  Lines       61618   61588     -30     
========================================
  Hits        40446   40446             
+ Misses      21172   21142     -30
@sagar-solana sagar-solana force-pushed the sagar-solana:fix_limit_ledger_stalls branch 4 times, most recently from e1c1138 to 640264f Dec 17, 2019
@sagar-solana

This comment has been minimized.

Copy link
Contributor Author

sagar-solana commented Dec 18, 2019

@sunnygleason this ran for hours with the compaction code manually picked up. It successfully limited the ledger size and I think it's good to go as soon as you have the compaction changes in.

// - A validator to download a snapshot from a peer and boot from it
// - To make sure that if a validator needs to reboot from its own snapshot, it has enough slots locally
// to catch back up to where it was when it stopped
pub const MAX_LEDGER_SLOTS: u64 = 6400;

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 18, 2019

Author Contributor

@mvines is this fine?

This comment has been minimized.

Copy link
@mvines

mvines Dec 18, 2019

Member

Sure, seems like the best value we can come up with now

@sagar-solana sagar-solana force-pushed the sagar-solana:fix_limit_ledger_stalls branch from 640264f to b7c79e3 Dec 18, 2019
@sunnygleason

This comment has been minimized.

Copy link
Collaborator

sunnygleason commented Dec 18, 2019

@sagar-solana I'm seeing really nice results with the compact_cf change (34MB vs 2GB previously)

Let me know when this is ready & I can benchmark it against the other scenarios!

@sagar-solana sagar-solana force-pushed the sagar-solana:fix_limit_ledger_stalls branch from b7c79e3 to e832e31 Dec 18, 2019
@mergify mergify bot dismissed pgarg66’s stale review Dec 18, 2019

Pull request has been modified.

if end {
match self.run_purge(from_slot, batch_end) {
Ok(end) => {
if let Err(e) = self.compact_storage(from_slot, batch_end) {

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 18, 2019

Author Contributor

@sunnygleason I had something more like this in mind. That combined function got way too large.

This comment has been minimized.

Copy link
@sunnygleason

sunnygleason Dec 18, 2019

Collaborator

@sagar-solana I think I understand, but it's not totally clear because this PR does a lot. Would you like me to take a shot at applying the minimal change in a smaller PR?

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 18, 2019

Author Contributor

Maybe I can explain. What is not clear?
I basically reintroduced your older function and just called it here. I prefer it to the merged function you introduced.
It's probably not a bad idea for you to review the rest of this pr as well.

This comment has been minimized.

Copy link
@sunnygleason

sunnygleason Dec 18, 2019

Collaborator

@sagar-solana sure thing! the main issue is that I can't benchmark this as cleanly because it's not the small/targeted change of delete vs delete_range

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 18, 2019

Author Contributor

I could give you a patch that does it with delete_slot if you want to compare but based on the docs, we shouldn't be doing a range scan and delete. Does that sound fair?

This comment has been minimized.

Copy link
@sunnygleason

sunnygleason Dec 18, 2019

Collaborator

sounds great!

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 18, 2019

Author Contributor

@sunnygleason #7549
Lets see how the perf changes :D

This comment has been minimized.

Copy link
@sagar-solana

sagar-solana Dec 18, 2019

Author Contributor

I've merged this one for now, if we see that delete_slot is faster for some reason, we can easily switch over to it via #7549

@sagar-solana sagar-solana merged commit 6a90056 into solana-labs:master Dec 18, 2019
12 checks passed
12 checks passed
Summary 1 rule matches and 3 potential rules
Details
buildkite/solana Build #16843 passed (34 minutes, 9 seconds)
Details
buildkite/solana/bench Passed (12 minutes, 57 seconds)
Details
buildkite/solana/checks Passed (2 minutes, 13 seconds)
Details
buildkite/solana/coverage Passed (8 minutes, 7 seconds)
Details
buildkite/solana/local-cluster Passed (22 minutes, 13 seconds)
Details
buildkite/solana/move Passed (4 seconds)
Details
buildkite/solana/pipeline-upload Passed (3 seconds)
Details
buildkite/solana/shellcheck Passed (32 seconds)
Details
buildkite/solana/stable Passed (31 minutes, 39 seconds)
Details
buildkite/solana/stable-perf Passed (10 minutes, 53 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
@sagar-solana sagar-solana deleted the sagar-solana:fix_limit_ledger_stalls branch Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.