Put the thread pool into the util directory #4756
Conversation
WalkthroughA lazily-initialized global Rayon thread pool ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Miner as Miner
participant Sync as Sync
participant Pool as commons/utils::thread_pool
rect rgb(235, 245, 255)
Note over Pool: Lazy-global Rayon pool (RAYON_EXEC_POOL)\ninitialized on first use via ThreadPoolBuilder
end
Miner->>Pool: import RAYON_EXEC_POOL\nspawn tasks via pool.spawn(...)
Pool-->>Miner: execute tasks (concurrent)
Sync->>Pool: import RAYON_EXEC_POOL\nspawn tasks via pool.spawn(...)
Pool-->>Sync: execute tasks (concurrent)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-02-10T10:00:57.422ZApplied to files:
📚 Learning: 2025-07-03T03:25:16.732ZApplied to files:
📚 Learning: 2025-08-08T10:27:43.881ZApplied to files:
📚 Learning: 2025-09-14T15:08:48.415ZApplied to files:
📚 Learning: 2025-08-08T10:20:45.797ZApplied to files:
📚 Learning: 2025-11-13T05:42:00.334ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sync/src/verified_rpc_client.rs (1)
868-870: Consider lowering this success log to debug.
get_blocksruns on every sync fetch; emitting aninfo!per success can flood logs under load. Dropping todebug!(or gating) keeps observability without overwhelming the pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
block-relayer/src/block_relayer.rs(2 hunks)commons/utils/Cargo.toml(1 hunks)commons/utils/src/lib.rs(1 hunks)commons/utils/src/thread_pool.rs(1 hunks)miner/Cargo.toml(1 hunks)miner/src/create_block_template/block_builder_service.rs(1 hunks)network/api/src/peer_provider.rs(1 hunks)sync/Cargo.toml(2 hunks)sync/src/announcement/mod.rs(3 hunks)sync/src/block_connector/execute_service.rs(1 hunks)sync/src/sync.rs(1 hunks)sync/src/verified_rpc_client.rs(2 hunks)sync/tests/verified_rpc_client_test.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
📚 Learning: 2025-09-01T03:56:58.362Z
Learnt from: welbon
Repo: starcoinorg/starcoin PR: 4633
File: vm/vm-runtime/Cargo.toml:48-48
Timestamp: 2025-09-01T03:56:58.362Z
Learning: In the Starcoin codebase, vm1 (starcoin-vm1-vm-runtime) may need to expose the same feature flags as vm2 to satisfy cross-version compatibility requirements when downstream projects like genesis depend on features from both versions. Empty feature declarations like `move-unit-test = []` may be intentionally added for compilation compatibility rather than to activate specific functionality.
Applied to files:
miner/Cargo.tomlsync/Cargo.toml
📚 Learning: 2025-11-13T05:42:00.334Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4755
File: network/api/src/peer_provider.rs:489-501
Timestamp: 2025-11-13T05:42:00.334Z
Learning: In network/api/src/peer_provider.rs, the select_peer_by_index method intentionally sorts the peer list on every invocation rather than caching the sorted list. This is because peer scores are updated concurrently via async operations, and caching would result in stale rankings. The design prioritizes correctness over performance, with the expectation that most calls succeed on the first attempt.
Applied to files:
block-relayer/src/block_relayer.rssync/src/verified_rpc_client.rsnetwork/api/src/peer_provider.rssync/src/announcement/mod.rs
📚 Learning: 2025-08-08T10:25:49.039Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/mock-service/src/lib.rs:114-120
Timestamp: 2025-08-08T10:25:49.039Z
Learning: In PR starcoinorg/starcoin#4605, txpool/mock-service/src/lib.rs: MockTxPoolService::next_sequence_number_with_header is currently unused; keeping todo!() in the mock is acceptable and won’t affect runtime unless invoked.
Applied to files:
sync/tests/verified_rpc_client_test.rs
📚 Learning: 2025-09-14T15:08:48.415Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Applied to files:
sync/tests/verified_rpc_client_test.rssync/src/block_connector/execute_service.rs
📚 Learning: 2025-08-08T10:20:45.797Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: chain/src/chain.rs:1104-1106
Timestamp: 2025-08-08T10:20:45.797Z
Learning: In starcoin/chain/src/chain.rs, ChainReader::get_block(hash) is intentionally implemented to return any block from storage without verifying membership in the current main chain/DAG view (the previous exist_block_filter was removed). Callers that require main-chain-only results should perform their own existence/membership checks (e.g., exist_block/check_exist_block) as needed.
Applied to files:
sync/tests/verified_rpc_client_test.rssync/src/verified_rpc_client.rssync/src/block_connector/execute_service.rs
📚 Learning: 2025-02-10T10:00:57.422Z
Learnt from: sanlee42
Repo: starcoinorg/starcoin PR: 4394
File: miner/src/lib.rs:259-263
Timestamp: 2025-02-10T10:00:57.422Z
Learning: The MinerService's task_pool is limited to 16 elements (DEFAULT_TASK_POOL_SIZE), making Vec a suitable data structure despite O(n) front removal operations.
Applied to files:
sync/src/block_connector/execute_service.rsminer/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-07-03T03:25:16.732Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4572
File: miner/src/create_block_template/new_header_service.rs:0-0
Timestamp: 2025-07-03T03:25:16.732Z
Learning: In Starcoin's miner/src/create_block_template/new_header_service.rs, panic! is intentionally used in impossible code branches (like equal block ID comparison after early equality check) to detect logical errors early and ensure immediate restart rather than allowing potentially corrupted state to continue.
Applied to files:
sync/src/block_connector/execute_service.rsminer/src/create_block_template/block_builder_service.rs
📚 Learning: 2025-10-20T09:58:31.897Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4722
File: chain/src/chain.rs:2151-2155
Timestamp: 2025-10-20T09:58:31.897Z
Learning: In starcoin/chain/src/chain.rs, for DAG block execution: the BlockChain instance's state (self.statedb.0 and self.statedb.1) must be initialized/positioned at the selected parent's state roots BEFORE execute_dag_block is called. Execution should validate and fail if state is not correctly positioned, rather than forking state mid-execution. This design avoids the performance overhead of repeated state forking during execution.
Applied to files:
sync/src/block_connector/execute_service.rs
📚 Learning: 2025-08-08T10:27:43.881Z
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4605
File: txpool/src/pool/queue.rs:389-392
Timestamp: 2025-08-08T10:27:43.881Z
Learning: In starcoinorg/starcoin#4605, txpool/src/pool/queue.rs: For PendingOrdering::Priority, if pool.try_read() fails, the design is to return an empty Vec and proceed with block building without waiting for the lock (non-blocking behavior is intentional).
Applied to files:
sync/src/block_connector/execute_service.rs
🧬 Code graph analysis (6)
block-relayer/src/block_relayer.rs (2)
config/src/sync_config.rs (1)
max_retry_times(56-58)sync/src/verified_rpc_client.rs (2)
new(39-44)new(115-124)
sync/tests/verified_rpc_client_test.rs (2)
sync/src/verified_rpc_client.rs (1)
rpc_result(802-820)node/src/lib.rs (1)
chain_service(164-166)
sync/src/sync.rs (1)
node/src/lib.rs (1)
config(128-132)
sync/src/verified_rpc_client.rs (2)
network/api/src/peer_provider.rs (3)
new(145-151)new(512-517)remaining(540-542)config/src/sync_config.rs (1)
max_retry_times(56-58)
network/api/src/peer_provider.rs (3)
sync/src/verified_rpc_client.rs (3)
new(39-44)new(115-124)selector(143-145)sync/src/tasks/mock.rs (4)
new(54-60)new(105-107)new(143-145)new(192-222)network/types/src/peer_info.rs (1)
peer_id(42-44)
sync/src/announcement/mod.rs (2)
node/src/lib.rs (1)
config(128-132)config/src/sync_config.rs (1)
max_retry_times(56-58)
🔇 Additional comments (16)
commons/utils/src/lib.rs (1)
23-23: LGTM! Thread pool module properly exported.The new
thread_poolmodule export cleanly extends the public API of the utils crate, aligning with the PR's objective to centralize thread pool management.sync/Cargo.toml (2)
51-51: LGTM! Dependency addition supports shared thread pool usage.The
sp-utilsdependency enables access to the externalized thread pool and aligns with the broader refactoring in this PR.
74-74: LGTM! Dev dependency addition for testing.The
starcoin-node-apidev-dependency appropriately supports test infrastructure.miner/Cargo.toml (1)
29-29: LGTM! Dependency addition enables shared thread pool access.The
sp-utilsdependency allows the miner module to leverage the centralized thread pool, consistent with the PR's refactoring goals.network/api/src/peer_provider.rs (2)
483-501: LGTM! Well-designed peer iteration API for retry logic.The
peer_iterator()andselect_peer_by_index()methods provide a clean interface for iterating through peers in score-descending order. The sorting on everyselect_peer_by_index()call is intentional to ensure fresh rankings given concurrent score updates. Based on learnings.
504-543: LGTM! PeerIterator implementation is clean and functional.The
PeerIteratorstruct provides a robust iteration mechanism for multi-peer retry scenarios. The helper methods (reset(),current_index(),remaining()) enhance usability. The use ofsaturating_addat line 524 is safe and prevents overflow, though reachingusize::MAXiterations is astronomically unlikely in practice.sync/src/announcement/mod.rs (2)
55-58: LGTM! Configuration retrieval follows established patterns.The code correctly retrieves
NodeConfigfrom the service context and derivesmax_retry_timesvia the config API, consistent with the pattern used elsewhere in the codebase.
74-77: LGTM! RPC client construction properly configured with retry parameter.The
max_retry_timesparameter is correctly threaded through toVerifiedRpcClient::new, enabling configurable retry behavior for RPC operations.commons/utils/Cargo.toml (1)
5-5: LGTM! Dependencies appropriately support thread pool implementation.The
num_cpusandrayondependencies are standard workspace dependencies that enable the new shared thread pool facility.num_cpusallows CPU-count-based pool sizing, whilerayonprovides the underlying thread pool implementation.Also applies to: 9-9
miner/src/create_block_template/block_builder_service.rs (2)
45-45: LGTM! External thread pool import is clean.The import correctly references the shared
RAYON_EXEC_POOLfrom thesp_utils::thread_poolmodule.
537-537: LGTM! Thread pool usage successfully migrated to external pool.The block template processing now leverages the shared
RAYON_EXEC_POOLinstead of a local thread pool, aligning with the PR's goal to centralize thread pool management. The spawn call signature and behavior remain unchanged.sync/src/sync.rs (1)
199-203: LGTM! RPC client construction updated with retry configuration.The
VerifiedRpcClient::newcall correctly includesconfig.sync.max_retry_times()as the third parameter, enabling configurable retry behavior. This change is consistent with the updated constructor signature and the pattern used throughout the PR.sync/src/block_connector/execute_service.rs (1)
21-21: Shared pool import keeps concurrency consistent.
Pulling the global executor fromsp_utilskeeps this service aligned with the shared thread pool strategy; no issues spotted.block-relayer/src/block_relayer.rs (1)
208-210: Propagating max_retry_times from config looks solid.
Fetching the value viaNodeConfigup front and threading it intoVerifiedRpcClient::newmatches the new constructor contract.Also applies to: 237-237
commons/utils/src/thread_pool.rs (1)
1-9: Global Rayon pool initialization looks good.
Centralizing the pool withLazyand CPU-bound sizing gives the rest of the crates a consistent executor.sync/tests/verified_rpc_client_test.rs (1)
101-144: Retry path test coverage looks great.
The mock verifies we fall back to the second peer and still surface peer attribution, which nicely guards the new retry loop.
jackzhhuang
force-pushed
the
thread-pool-for-block
branch
from
4adbf68 to
15effc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
commons/utils/Cargo.toml (1)
5-5: Workspace dependencies confirmed; consider optional stdlib alternative for num_cpus.The
num_cpusandrayondependencies are correctly defined in the root workspaceCargo.toml(lines 453 and 473 respectively). Both versions carry no known security advisories.Since your MSRV is Rust 1.86, you can optionally replace
num_cpuswithstd::thread::available_parallelism()(stable since Rust 1.59) to reduce dependencies. However, this is not required—v1.13 is actively maintained.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
commons/utils/Cargo.toml(1 hunks)commons/utils/src/lib.rs(1 hunks)commons/utils/src/thread_pool.rs(1 hunks)miner/Cargo.toml(1 hunks)miner/src/create_block_template/block_builder_service.rs(1 hunks)sync/Cargo.toml(1 hunks)sync/src/block_connector/execute_service.rs(1 hunks)sync/tests/verified_rpc_client_test.rs(0 hunks)
💤 Files with no reviewable changes (1)
- sync/tests/verified_rpc_client_test.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- commons/utils/src/thread_pool.rs
- sync/Cargo.toml
- miner/Cargo.toml
- miner/src/create_block_template/block_builder_service.rs
- sync/src/block_connector/execute_service.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sanlee42
Repo: starcoinorg/starcoin PR: 4394
File: miner/src/lib.rs:259-263
Timestamp: 2025-02-10T10:00:57.422Z
Learning: The MinerService's task_pool is limited to 16 elements (DEFAULT_TASK_POOL_SIZE), making Vec a suitable data structure despite O(n) front removal operations.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4648
File: miner/tests/miner_test.rs:87-93
Timestamp: 2025-09-14T15:08:48.415Z
Learning: In Starcoin test files (like miner/tests/miner_test.rs), jackzhhuang prefers to use std::thread::sleep for intentional blocking delays in test scenarios, even within actor event handlers, rather than using non-blocking ctx.run_later alternatives.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
commons/utils/src/thread_pool.rs (2)
3-9: Consider making the thread pool size configurable.The thread pool is hardcoded to use all CPU cores, which may not be optimal for all workloads or deployment scenarios. Consider adding configurability via environment variables or a configuration parameter to allow tuning based on workload characteristics.
For example, you could add an environment variable override:
pub static RAYON_EXEC_POOL: Lazy<rayon::ThreadPool> = Lazy::new(|| { + let num_threads = std::env::var("RAYON_EXEC_POOL_THREADS") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or_else(num_cpus::get); + rayon::ThreadPoolBuilder::new() - .num_threads(num_cpus::get()) + .num_threads(num_threads) .thread_name(|index| format!("parallel_executor_{}", index)) .build() .expect("failed to build global rayon thread pool for parallel execution") });
3-9: Be mindful of resource contention with a shared global pool.Since this pool is now shared between the miner and sync services (previously each had their own), be aware that heavy parallel workloads from one service could starve the other. Monitor pool utilization and task queue depths in production to ensure balanced resource allocation.
If contention becomes an issue, consider:
- Separate pools for different workload types (CPU-bound vs I/O-bound)
- Priority-based task scheduling
- Dedicated pools per service with configurable sizes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
commons/utils/src/thread_pool.rs(1 hunks)miner/src/create_block_template/block_builder_service.rs(1 hunks)sync/src/block_connector/execute_service.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sync/src/block_connector/execute_service.rs
- miner/src/create_block_template/block_builder_service.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sanlee42
Repo: starcoinorg/starcoin PR: 4394
File: miner/src/lib.rs:259-263
Timestamp: 2025-02-10T10:00:57.422Z
Learning: The MinerService's task_pool is limited to 16 elements (DEFAULT_TASK_POOL_SIZE), making Vec a suitable data structure despite O(n) front removal operations.
Learnt from: jackzhhuang
Repo: starcoinorg/starcoin PR: 4698
File: cmd/tx-factory/src/main.rs:165-165
Timestamp: 2025-09-28T08:35:37.355Z
Learning: In the tx-factory cmd tool, jackzhhuang prefers long unlock durations (multiple hours) for account unlocking rather than the typical short durations like 1 minute.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build and test
🔇 Additional comments (1)
commons/utils/src/thread_pool.rs (1)
1-2: LGTM!The use of
once_cell::sync::Lazyis appropriate for lazy initialization of the global thread pool.
jackzhhuang
force-pushed
the
thread-pool-for-block
branch
from
57937c2 to
41afb99
Compare
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit
Refactor
Chores
Tests