perf(tree): worker pooling for storage in multiproof generation #18883
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yongkangc
changed the title
yongkangc
changed the title
- Introduced constants for maximum and minimum storage/account proof workers to manage memory usage. - Implemented `default_proof_workers` function to calculate optimal worker counts based on available CPU cores. - Updated `TreeConfig` to include storage and account proof worker counts, with defaults derived from CPU detection. - Modified `ProofTaskManager` initialization to accept storage worker count. - Adjusted tests to accommodate new proof worker configurations.
- Simplified the `ProofTaskManager` by implementing a transaction pool using `crossbeam` channels for better concurrency. - Replaced the previous transaction management logic with a pre-initialized pool of transactions, improving efficiency in handling proof tasks. - Updated task dispatching to utilize the new transaction pool, ensuring smoother execution and resource management. - Enhanced error handling and logging for transaction operations to improve debugging capabilities.
- Changed `account_nodes` and `storage_nodes` from `usize` to `AtomicUsize` to support concurrent updates from multiple workers. - Implemented a custom `Clone` for `ProofTaskMetrics` to handle atomic values correctly. - Updated the `record` method to load atomic values with `Ordering::Relaxed` for improved performance.
- Replaced `ProofTaskManager` with a new `new_proof_task_handle` function for improved task management. - Updated related code to utilize the new proof task handle, enhancing clarity and reducing complexity. - Adjusted tests to reflect changes in proof task initialization and handling.
yongkangc
force-pushed
the
yk/worker_pool_manager
branch
from
c4cf477 to
3ee7c73
Compare
- Updated the `spawn` method in `payload_processor` to return a `ProviderResult`, improving error handling during task initialization. - Refactored related code in `payload_validator` and `multiproof` to handle the new result type, ensuring consistent error management. - Adjusted tests to validate the changes in task spawning and error handling mechanisms.
- Consolidated the initialization of `StateProviderBuilder` in the `bench_state_root` function for improved readability. - Streamlined the `spawn` method calls in `payload_validator` and `payload_processor` to enhance code clarity. - Adjusted formatting and structure in `proof_task` to maintain consistency across the codebase.
- Added logic to drain receivers for accounts not touched by the walker, allowing workers to deliver results without encountering closed channels. - Enhanced error handling for closed channels during storage proof reception, ensuring robustness in parallel processing.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors proof generation to use worker pooling for improved performance. It replaces the existing ProofTaskManager with a new new_proof_task_handle function that creates a pool of pre-warmed database transactions.
- Replaced
ProofTaskManagerwith direct worker pooling via crossbeam channels - Added atomic metrics tracking for concurrent access
- Introduced storage and account worker count configuration with auto-detection based on CPU cores
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/trie/parallel/src/proof_task_metrics.rs | Updated metrics to use atomic types for thread-safe concurrent access |
| crates/trie/parallel/src/proof_task.rs | Complete refactor replacing ProofTaskManager with worker pool architecture |
| crates/trie/parallel/src/proof.rs | Updated to use crossbeam channels and handle orphaned receivers |
| crates/trie/parallel/Cargo.toml | Added crossbeam-channel dependency |
| crates/engine/tree/src/tree/payload_validator.rs | Added error handling for proof task creation |
| crates/engine/tree/src/tree/payload_processor/multiproof.rs | Updated test to use new proof task handle creation |
| crates/engine/tree/src/tree/payload_processor/mod.rs | Updated to use new proof task handle and added error handling |
| crates/engine/tree/benches/state_root_task.rs | Added error handling for payload processor spawn |
| crates/engine/primitives/src/config.rs | Added worker count configuration with CPU-based auto-detection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
67
to
70
| let max_concurrency = max_concurrency.max(1); | ||
| let storage_worker_count = storage_worker_count.max(1); | ||
| let queue_capacity = max_concurrency; | ||
| let worker_count = storage_worker_count.min(max_concurrency); |
There was a problem hiding this comment.
The relationship between max_concurrency, storage_worker_count, and the final worker_count is unclear. The logic suggests worker_count is capped by max_concurrency, but the purpose of having both parameters is not evident from the code or documentation.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
- Updated the documentation for `new_proof_task_handle` to clarify its purpose and functionality. - Refined the logic for determining `queue_capacity` and `worker_count` to ensure proper initialization of the worker pool. - Removed outdated comments regarding transaction pool resource management for clarity.
- Eliminated constants and functions related to storage and account proof worker counts to simplify configuration. - Updated `TreeConfig` and related functions to remove references to proof worker counts, streamlining the codebase. - Adjusted proof task handling to utilize half of the queue capacity for storage proofs, enhancing resource management.
yongkangc
commented
Oct 7, 2025
| /// A task that manages sending multiproof requests to a number of tasks that have longer-running | ||
| /// database transactions | ||
| #[derive(Debug)] | ||
| pub struct ProofTaskManager<Factory: DatabaseProviderFactory> { |
There was a problem hiding this comment.
debated really hard about removing prooftask manager in this pr - thought it makes things much easier to wire up rn
because the prooftasksmanager had too many previous abstractions that were uncessary and only used by storage proof, it was easier to remove that now
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
initial benching at commit 6d117dd |
|
moved over to #18887 |
Latest commit seems to have improved slightly after messing with the workers config |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to add worker pooling to our existing proof generation. This is the first pr to perform this migration.
Core logic:
Changes:
ProofTaskManagerwith a newnew_proof_task_handlefunction for improved task management. We removed ProofTaskManager here as it was an intemediary that is not needed with direct wiring.Issues closed:
ProofTaskManager#18735References:
Next Steps: