feat: unified EVM worker pool - #2710
gventino-cw wants to merge 7 commits into
Conversation
Replace the per-kind EVM worker pools (call-present, call-past, inspector) with a single pool shared by every execution kind, closing #2708. - One set of `executor.evm_workers` OS threads (default 150) pulling from a single bounded channel; each worker's EVM executes any kind, since the point-in-time is selected per task by its input. - Per-kind concurrency limits (`executor.call_present_limit`, `executor.call_past_limit`, `executor.inspector_limit`) enforced by counting semaphores acquired on the sender side: the permit rides inside the task and is released after execution, so no kind can be head-of-line blocked by unrelated kinds and FIFO fairness is preserved. - `call_present_limit` defaults to the pool capacity remaining after the other kinds, so the busiest kind can use every idle worker. - `executor.evm_flex_quota` (default 0) adds extra permits any saturated kind can borrow, sharing idle capacity across kinds. - Deprecated `executor.{call_present,call_past,inspector}_evms` fields are aliases that preserve the total capacity of pre-migration deployments. - Generalized `Semaphore` with `try_acquire` and shutdown-aware acquire (condvar polling), with pluggable metrics; new pool gauges `executor_workers_total`, `executor_pool_permits_waiting`, `executor_pool_permits_held` and `executor_pool_queue_len`. - De-genericized `Evm` (the `Input` parameter was PhantomData-only) and moved the worker loop into `EvmWorkerPool::worker`.
|
Failed to generate code suggestions for PR |
|
Benchmark: Git Info:
Leader Stats: Follower Stats: Plots: |
compared this with this (it seems ok but the std dev increase it's kinda sus): |
![cloudwalk-review-agent[bot]](https://avatars.githubusercontent.com/u/6581007?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Nice refactor overall — the unified pool + per-kind semaphores is a meaningful improvement and the config-resolution tests are solid. I found one blocking correctness issue around task submission error handling that can leak permits and return the wrong RPC outcome for inspect calls.
Blocking finding
The inspect() path ignores send failures (let _ = self.tx.send(task);). If the worker queue/channel is closed, this still waits on inspector_rx.recv() and then reports ChannelClosed from the oneshot side instead of surfacing the real enqueue failure. More importantly, because the Permit is moved into PoolTask and the task is dropped on send error, this path should fail immediately and deterministically, mirroring execute() behavior.
Please handle self.tx.send(task)? (or equivalent mapped error) in inspect() as done in execute(), and only wait on the oneshot when enqueue succeeds.
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Summary
Good refactor direction overall (unified pool + admission control), but I found one blocking correctness issue still present in the updated code path.
Blocking finding
inspect() still ignores queue send failures (let _ = self.tx.send(task);). If the pool channel is closed/full-erroring, the function proceeds to wait on the oneshot and eventually returns ChannelClosed from the response side instead of surfacing the real enqueue failure immediately. This also diverges from execute() behavior, which correctly propagates self.tx.send(task)?.
Please handle send failure in inspect() the same way as execute() (propagate/convert the enqueue error before waiting on inspector_rx.recv()).
|
Benchmark: Git Info:
Leader Stats: Follower Stats: Plots: |
There was a problem hiding this comment.
Summary
Blocking issue found in the new unified pool admission path: the global inflight_total counter is updated with a non-atomic check-then-increment sequence, which allows concurrent acquires to over-admit through the relaxed path well past the configured busy threshold. This breaks the intended transition point where per-kind throttling should start and can materially exceed configured concurrency controls under load.
I did not find other concrete correctness/security issues in the provided diff context.
There was a problem hiding this comment.
Summary
I reviewed the unified EVM pool refactor, including admission control, task routing, config migration surface, and concurrency behavior in the provided diff context. The previously reported over-admission race (inflight_total check-then-increment) is now addressed by moving per-kind admission accounting behind mutexed gates and using a single atomic total counter update after successful admission.
The new pool admission logic appears internally consistent (permit lifecycle via RAII, throttled wait with shutdown escape hatch, and per-kind metrics updates), and the EVM de-genericization changes are propagated through executor and transaction worker call sites without obvious type/behavioral regressions.
Config and validation changes are coherent with the new model (flattened PoolConfig, bounded clap ranges, warning-level cross-field validation), and there are focused tests for pool defaults/limits and config-loader merge semantics.
No concrete blocking correctness or security issues were found in the supplied patch/context.
| type Input = Input; | ||
| /// Returns the pool kind of a call: calls against the latest state and calls against a past state | ||
| /// are admitted by different pool gates. | ||
| fn call_evm_kind(input: &CallExecutionInput) -> EvmKind { |
There was a problem hiding this comment.
I think we should rethink EvmKind, ExecutionKind and PoolTaskKind (and maybe pointintime and blockfilter too (and EvmRoute before you removed it)). It seems to me the typing is redundant and loses its meaning sometimes.
For instance, a CallPast execution kind always maps onto a CallPast evm kind, an ExecutionKind::CallPresent maps onto EvmKind::CallPresent etc. Then why have EvmKind at all yk? At best the mappings stay consistent but redundant, at worst we eventually have a CallPast execution running on an Inspect EvmKind, passed on a CallPresent pool task.
I tried reworking this a bit in some of the previous PRs I merged but it still is not there, but since this PR is building on top of it I think we should address this first.
I'll try thinking of something but tell me if you have any good ideas of how we should structure this too.
There was a problem hiding this comment.
I always got blockfilter and point in time concepts mixed up, so yes, I think it's fair to rethink these (and the other ones too)
For instance, a CallPast execution kind always maps onto a CallPast evm kind, an ExecutionKind::CallPresent maps onto EvmKind::CallPresent etc. Then why have EvmKind at all yk?
yep, it makes a lot of sense.
I tried reworking this a bit in some of the previous PRs I merged but it still is not there, but since this PR is building on top of it I think we should address this first. I'll try thinking of something but tell me if you have any good ideas of how we should structure this too.
sure, i agree with u and think that it's completely fair. I need to think about it, will try to open another pr with an alternative
This branch has not been deployed
2 participants
Closes #2708
Summary
Replaces the per-kind EVM worker pools (call-present, call-past, inspector) with a single unified pool. Configs become per-kind concurrency limits enforced by counting semaphores, so the scenario of hundreds of idle workers in one kind while another kind is queued no longer happens.
Changes
executor.evm_workersOS threads (default 150) pullingPoolTasks from a single bounded channel (4096). Each worker'sEvmexecutes any kind — the point-in-time of each task is selected by its own input.executor.call_present_limit,executor.call_past_limitandexecutor.inspector_limitcap concurrent in-flight tasks per kind. Permits are acquired on the sender side (own limit, then shared flex quota, then blocking on the own limit with a shutdown-aware condvar wait) and ride inside the task, released after execution — no head-of-line blocking, FIFO per kind.call_present_limitfalls back to the pool capacity remaining after the other kinds' limits, so the busiest kind can use every idle worker.executor.evm_flex_quota(default 0) adds extra permits that any saturated kind can borrow, sharing idle capacity across kinds. A value around 20% of the pool is the suggested starting point after soak testing.executor.call_present_evms/call_past_evms/inspector_evmsfields are deprecated aliases; when only they are set, total workers = their sum, preserving pre-migration capacity. A warning is logged when they are used.sum(limits) > workersfails at startup (ExecutorConfig::initnow returnsanyhow::Result).src/utils.rs):try_acquire,acquire_shutdown_aware(condvar polling ofGlobalState::is_shutdown), and pluggable metrics — the local transaction warmup semaphore keeps its legacy unlabeled metrics, other semaphores stay metric-less.executor_workers_total,executor_pool_permits_waiting{kind},executor_pool_permits_held{kind}andexecutor_pool_queue_len;executor_workers_busy{pool}is now labeled by the task's kind since workers are shared.Evmde-genericization: the<Input>type parameter wasPhantomData-only;Evm::executeis now generic over the input argument instead, andinspectis inherent.TransactionWorker(unchanged — they were never part of the pooled kinds).Notes for reviewers
Verification
cargo check --all-targets✅cargo +nightly-2026-05-08 fmt --all✅-D warnings) ✅cargo test --lib— 232 passed ✅ (includes 7 newPoolConfig::resolvetests)cargo test --test config_loader— 11 passed ✅