perf(l1): set high thread priority on block execution pipeline threads#6300
perf(l1): set high thread priority on block execution pipeline threads#6300ilitteri wants to merge 2 commits into
Conversation
…hanges The rebase onto main broke this branch because it had warmer-cancellation code mixed in (cancelled AtomicBool, extra params to warm_block). Remove those changes since they belong to perf/warmer-cancellation, and fix tracing::debug! to use the already-imported debug! macro.
github-actions
Bot
added
L1
🤖 Kimi Code ReviewReview SummaryThe PR introduces thread priority management for block processing threads. While the intent is reasonable (improving performance during sync), there are several issues that need addressing: Critical Issues
Security Considerations
Suggested Changes// blockchain.rs:225-240
fn set_block_processing_priority() -> Result<(), String> {
use thread_priority::{ThreadPriority, ThreadPriorityValue};
// Make priority configurable
let priority = std::env::var("ETHREX_THREAD_PRIORITY")
.ok()
.and_then(|s| s.parse::<u8>().ok())
.map(|p| ThreadPriority::Crossplatform(
ThreadPriorityValue::try_from(p).unwrap_or(ThreadPriorityValue::MAX)
))
.unwrap_or(ThreadPriority::Max);
priority.set_for_current()
.map_err(|e| format!("Failed to set thread priority: {}", e))
}
// Then use it with proper error handling:
if let Err(e) = set_block_processing_priority() {
warn!("{}", e);
}Additional Recommendations
The current implementation is too aggressive and could cause issues in production environments. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code Review
No EVM opcode/gas/state-transition logic changed in this diff; concerns are operational robustness and security posture around thread scheduling. Automated review by OpenAI Codex · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThis PR elevates OS thread priority for the three block execution pipeline threads ( Key changes:
One style note: The Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| crates/blockchain/blockchain.rs | Adds increase_thread_priority() helper and calls it at the start of all three pipeline thread closures (warmer, execution, merkleizer). Logic is sound; minor style issue: expect on the fallback ThreadPriorityValue::try_from(62u8) is inconsistent with the function's design to silently ignore all failures. |
| Cargo.toml | Adds thread-priority = "3.0.0" to the workspace dependency manifest. Version is pinned; placement is appropriate. |
| crates/blockchain/Cargo.toml | Wires thread-priority.workspace = true into the blockchain crate's dependencies. No issues. |
Sequence Diagram
sequenceDiagram
participant Caller as Blockchain::add_block
participant Scope as thread::scope
participant W as block_executor_warmer
participant E as block_executor_execution
participant M as block_executor_merkleizer
participant OS as OS Scheduler
Caller->>Scope: spawn scoped threads
Scope->>W: spawn
W->>OS: increase_thread_priority()<br/>(Max → fallback 62/100)
OS-->>W: priority elevated (or silently ignored)
W->>W: warm_block / warm_block_from_bal
Scope->>E: spawn
E->>OS: increase_thread_priority()<br/>(Max → fallback 62/100)
OS-->>E: priority elevated (or silently ignored)
E->>M: send merkleization requests (channel)
E->>E: execute_block_pipeline + validations
Scope->>M: spawn
M->>OS: increase_thread_priority()<br/>(Max → fallback 62/100)
OS-->>M: priority elevated (or silently ignored)
M->>M: handle_merkleization / handle_merkleization_bal
W-->>Scope: warmer_duration
E-->>Scope: (execution_result, exec_end_instant)
M-->>Scope: (account_updates_list, accumulated_updates, merkle_end_instant)
Scope-->>Caller: (execution_result, merkleization_result, warmer_duration)
Last reviewed commit: 3796e75
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let fallback = ThreadPriority::Crossplatform( | ||
| ThreadPriorityValue::try_from(62u8).expect("62 is within valid 0..100 range"), | ||
| ); | ||
| if let Err(err) = fallback.set_for_current() { | ||
| debug!(?err, "failed to set moderate thread priority"); | ||
| } |
There was a problem hiding this comment.
expect inconsistent with best-effort error-handling philosophy
increase_thread_priority is designed to silently absorb all failures (errors are logged at debug level and ignored), but the inner expect introduces a panic path. Although 62u8 is a compile-time constant that will always satisfy the 0..=99 range check, the use of expect is semantically inconsistent with the function's intent. If the fallback value were ever changed to an out-of-range constant, or if a future platform changes the valid range, this would panic in production code that is supposed to be non-critical.
Consider handling the try_from result the same way the rest of the function does:
| let fallback = ThreadPriority::Crossplatform( | |
| ThreadPriorityValue::try_from(62u8).expect("62 is within valid 0..100 range"), | |
| ); | |
| if let Err(err) = fallback.set_for_current() { | |
| debug!(?err, "failed to set moderate thread priority"); | |
| } | |
| let fallback = match ThreadPriorityValue::try_from(62u8) { | |
| Ok(val) => ThreadPriority::Crossplatform(val), | |
| Err(err) => { | |
| debug!(?err, "invalid fallback thread priority value"); | |
| return; | |
| } | |
| }; | |
| if let Err(err) = fallback.set_for_current() { | |
| debug!(?err, "failed to set moderate thread priority"); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/blockchain.rs
Line: 232-237
Comment:
**`expect` inconsistent with best-effort error-handling philosophy**
`increase_thread_priority` is designed to silently absorb all failures (errors are logged at `debug` level and ignored), but the inner `expect` introduces a `panic` path. Although `62u8` is a compile-time constant that will always satisfy the `0..=99` range check, the use of `expect` is semantically inconsistent with the function's intent. If the fallback value were ever changed to an out-of-range constant, or if a future platform changes the valid range, this would panic in production code that is supposed to be non-critical.
Consider handling the `try_from` result the same way the rest of the function does:
```suggestion
let fallback = match ThreadPriorityValue::try_from(62u8) {
Ok(val) => ThreadPriority::Crossplatform(val),
Err(err) => {
debug!(?err, "invalid fallback thread priority value");
return;
}
};
if let Err(err) = fallback.set_for_current() {
debug!(?err, "failed to set moderate thread priority");
}
```
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewHere is the code review for PR #6300: Review:
|
| Design | Good — best-effort with clean fallback |
| Correctness | Correct for the threads it covers |
| Gap | Worker sub-threads (shard_worker, bal_storage_worker, bal_state_shard) should also call increase_thread_priority() for consistent coverage |
| Usability | Consider documenting the CAP_SYS_NICE requirement so operators know when to expect this to take effect |
Automated review by Claude (Anthropic) · custom prompt
|
Recreating with updated description. |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce block import latency under CPU contention by elevating OS thread priority for the L1 block execution pipeline’s scoped threads (warmer, executor, merkleizer).
Changes:
- Adds the
thread-prioritycrate as a workspace dependency and enables it inethrex-blockchain. - Introduces an
increase_thread_priority()helper that attempts max priority with a cross-platform fallback. - Calls the helper at the start of the warmer/execution/merkleizer scoped threads.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/blockchain/blockchain.rs |
Adds the priority-bump helper and applies it to the three pipeline threads. |
crates/blockchain/Cargo.toml |
Adds thread-priority to the blockchain crate dependencies via workspace. |
Cargo.toml |
Adds thread-priority to [workspace.dependencies] with a pinned version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| indexmap = { version = "2.11.4" } | ||
| k256 = "0.13.4" | ||
| anyhow = "1.0.86" | ||
| thread-priority = "3.0.0" |
There was a problem hiding this comment.
thread-priority was added to workspace dependencies, but Cargo.lock was not updated (it currently has no thread-priority entries). CI runs make check-cargo-lock (uses cargo metadata --locked), so this will fail unless you regenerate and commit the updated lockfile(s) (e.g., via make update-cargo-lock).
| thread-priority = "3.0.0" |
| let fallback = ThreadPriority::Crossplatform( | ||
| ThreadPriorityValue::try_from(62u8).expect("62 is within valid 0..100 range"), | ||
| ); |
There was a problem hiding this comment.
This helper is intended to be a non-critical optimization that logs and continues on failure, but it contains an expect(...) when building the fallback priority value. Please avoid panicking here; handle the try_from result explicitly (and log/return if it ever fails) so thread priority adjustment can never crash the pipeline threads.
| let fallback = ThreadPriority::Crossplatform( | |
| ThreadPriorityValue::try_from(62u8).expect("62 is within valid 0..100 range"), | |
| ); | |
| let fallback_value = match ThreadPriorityValue::try_from(62u8) { | |
| Ok(value) => value, | |
| Err(err) => { | |
| debug!(?err, "failed to build fallback thread priority value"); | |
| return; | |
| } | |
| }; | |
| let fallback = ThreadPriority::Crossplatform(fallback_value); |
Motivation
The block execution pipeline (warmer, executor, merkleizer) runs on scoped threads that compete with background work (networking, syncing, RocksDB compaction) for CPU time. Setting high thread priority ensures these latency-critical threads get scheduled preferentially by the OS.
Description
thread-prioritycrate dependencyincrease_thread_priority()helper that attempts max priority with a fallback to priority 62/100block_executor_warmer,block_executor_execution, andblock_executor_merkleizerHow to Test
cargo checkpassesps -eo pid,ni,comm | grep block_executor