8000 blockstore: Adjust rocksdb threadpool config by steviez · Pull Request #6166 · anza-xyz/agave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

blockstore: Adjust rocksdb threadpool config #6166

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 9, 2025
Merged

blockstore: Adjust rocksdb threadpool config #6166

merged 1 commit into from
May 9, 2025

Conversation

steviez
Copy link
@steviez steviez commented May 8, 2025 •
edited
Loading

Problem

A previous change wired up the ability to configure rocksdb threadpool sizes through BlockstoreOptions. That change kept the threadpool sizes the same, but unintentionally limited how many concurrent flushes and compactions could be scheduled in each pool. The result was reduced parallelism with mostly idle threadpools. The previous change is #4214

Summary of Changes

This change restores the previous level of allowed parallelism in the compact and flush threadpools

Who is Effected

With current MNB load, I believe the node had to have the following in order to experience a noticeable issue:

  1. Running with --enable-rpc-transaction-history
  2. Running with larger than default --limit-ledger-size
  3. Potentially getting actual RPC load on Blockstore methods; these increase reads which compete with compaction

Item 1. introduces two extra columns that increase our total amount of data written to the Blockstore by 2-3x. Item 2. opens up to some elevated I/O to clean those two extra columns (see #6017 for more details).

Testing

With the current tip of master, I put a breakpoint here, and observed the following (noting that the pool sizes scale with N threads):

// v2.1
(gdb) print res
$1 = {max_flushes
8000
 = 8, max_compactions = 24}

// tip of master & v2.2
(gdb) print res
$1 = {max_flushes = 1, max_compactions = 1}

// this PR
(gdb) print res
$1 = {max_flushes = 8, max_compactions = 32}

You may notice that max_compaction changes in this PR vs. v2.1. In rocks, the size of the threadpool and the number of jobs that can be scheduled in parallel are separate numbers:

  • In <= v2.1 on this box, we'd create 32 threads for compaction but only allow up to 24 jobs to be run in parallel
  • This PR allows all 32 created threads to be utilized at once
    • I'd argue this makes the configuration more intuitive, and we can reduce the threadpool size subsequently

@steviez
blockstore: Adjust rocksdb threadpool config
87fe4ae 
A previous change wired up the ability to configure rocksdb threadpool
sizes through BlockstoreOptions. That change kept the threadpool sizes
the same, but unintentionally limited how many concurrent flushes and
compactions could be scheduled in each pool. The result was reduced
parallelism with mostly idle threadpools.

This change restores the previous level of allowed parallelism in the
compact and flush threadpools
@steviez
Copy link
Author
steviez commented May 8, 2025 •
edited
Loading

I'm also hoping to get some runtime with this patch from some people who have seen issues with v2.2.

Assuming I have correctly identified the issue, we will need this for v2.2 as well. I see two options:

@t-nelson
Copy link
t-nelson commented May 8, 2025

I'm also hoping to get some runtime with this patch from some people who have seen issues with v2.2.

Assuming I have correctly identified the issue, we will need this for v2.2 as well. I see two options:

* Merge this PR and BP it to v2.2

* Revert [validator: Add CLI args to control rocksdb threadpool sizes #4214](https://github.com/anza-xyz/agave/pull/4214) and BP the revert to v2.2

are there any non-obvious concerns related to bping this pr? seems to be (mostly) reverting us to 2.1 behavior

@steviez
Copy link
Author
steviez commented May 8, 2025

are there any non-obvious concerns related to bping this pr?

No, not really. I'm technically using a different function to modify the config, but I have confidence it is doing the same thing that it was before via the gdb inspection + calling ps -p PID -T to examine created threads

seems to be (mostly) reverting us to 2.1 behavior

Yeah, more or less. Also leave the CLI args hooked up which aren't in v2.1

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.8%. Comparing base (a1f90ac) to head (87fe4ae).
Report is 3 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff            @@
##           master    #6166     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         842      842             
  Lines      377696   377690      -6     
=========================================
- Hits       313061   313048     -13     
- Misses      64635    64642      +7
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@t-nelson t-nelson added the v2.2 Backport to v2.2 branch label May 9, 2025
@mergify Mergify
Copy link
mergify bot commented May 9, 2025

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

8000
@steviez steviez merged commit 93df405 into anza-xyz:master May 9, 2025
48 checks passed
@steviez steviez deleted the bstore_fix_bg_jobs branch May 9, 2025 15:29
mergify bot pushed a commit that referenced this pull request May 9, 2025
@steviez @mergify
blockstore: Adjust rocksdb threadpool config (#6166)
fe7c145 
A previous change wired up the ability to configure rocksdb threadpool
sizes through BlockstoreOptions. That change kept the threadpool sizes
the same, but unintentionally limited how many concurrent flushes and
compactions could be scheduled in each pool. The result was reduced
parallelism with mostly idle threadpools.

This change restores the previous level of allowed parallelism in the
compact and flush threadpools

(cherry picked from commit 93df405)
@mergify mergify bot mentioned this pull request May 9, 2025
Merged
steviez added a commit that referenced this pull request May 9, 2025
@mergify @steviez
v2.2: blockstore: Adjust rocksdb threadpool config (backport of #6166) (
e2b5776 
#6179)

blockstore: Adjust rocksdb threadpool config (#6166)

A previous change wired up the ability to configure rocksdb threadpool
sizes through BlockstoreOptions. That change kept the threadpool sizes
the same, but unintentionally limited how many concurrent flushes and
compactions could be scheduled in each pool. The result was reduced
parallelism with mostly idle threadpools.

This change restores the previous level of allowed parallelism in the
compact and flush threadpools

(cherry picked from commit 93df405)

Co-authored-by: steviez <steven@anza.xyz>
nibty pushed a commit to x1-labs/tachyon that referenced this pull request Jun 11, 2025
@mergify @steviez @nibty
v2.2: blockstore: Adjust rocksdb threadpool config (backport of anza-…
56b1adc 
…xyz#6166) (anza-xyz#6179)

blockstore: Adjust rocksdb threadpool config (anza-xyz#6166)

A previous change wired up the ability to configure rocksdb threadpool
sizes through BlockstoreOptions. That change kept the threadpool sizes
the same, but unintentionally limited how many concurrent flushes and
compactions could be scheduled in each pool. The result was reduced
parallelism with mostly idle threadpools.

This change restores the previous level of allowed parallelism in the
compact and flush threadpools

(cherry picked from commit 93df405)

Co-authored-by: steviez <steven@anza.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-nelson t-nelson t-nelson approved these changes

Assignees
No one assigned
Labels
v2.2 Backport to v2.2 branch
Projects
Thread and Thread Pool Audit
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@steviez @t-nelson @codecov-commenter
0