perf: move provider to background thread - #1486
Conversation
GitHub Actions
Inactive
|
GitHub Actions
Failure
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1486 +/- ##
==========================================
- Coverage 79.89% 75.93% -3.96%
==========================================
Files 452 452
Lines 78956 79068 +112
Branches 78956 79068 +112
==========================================
- Hits 63078 60043 -3035
- Misses 13697 16942 +3245
+ Partials 2181 2083 -98 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
65b08b7 to
3f812fe
Compare
GitHub Actions
Error
3f812fe to
c689632
Compare
GitHub Actions
Inactive
|
/bench |
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
/bench |
|
🚀 Starting regression benchmark for |
|
/bench |
|
🚀 Starting regression benchmark for |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Failure
|
/bench hardhat-ref=ci/fix-outdated-deps |
|
🚀 Starting regression benchmark for |
|
/bench hardhat-ref=ci/fix-outdated-deps |
|
🚀 Starting regression benchmark for |
|
/bench |
|
🚀 Starting regression benchmark for |
|
✅ Regression benchmark passed for |
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Failure
Replace the per-request `spawn_blocking` + blocking channel round trip with a completion callback: `handle_request` now deserializes the request on the calling thread, enqueues it on the provider's background thread, and settles the napi deferred from that thread once the response is available. This cuts the thread handoffs per request from four to two (~7x fewer futex syscalls), making the provider benchmark suite ~2x faster than main. Queued requests are settled with the new `ProviderError::UnexpectedTermination` when the provider shuts down, so pending promises always resolve.
|
/bench |
There was a problem hiding this comment.
Pull request overview
Refactors edr_provider to run all JSON-RPC request handling (and interval mining) on a dedicated background OS thread, replacing mutex-based synchronization and tokio blocking with channel-based message passing. This aims to improve safety/latency on callers (notably the JS main thread) by ensuring requests are serialized off-thread and surfaced via callbacks.
Changes:
- Introduce a new
backendevent loop running on a dedicatedCancellableThread, processing requests viacrossbeam-channeland integrating interval mining scheduling. - Update provider APIs and N-API bindings to enqueue requests and resolve responses asynchronously (callback/promise-based) instead of blocking execution on the JS thread.
- Add integration coverage for interval mining enable/disable behavior and adjust trait bounds for Send requirements.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/edr_provider/tests/integration/mod.rs | Registers the new interval mining integration test module. |
| crates/edr_provider/tests/integration/interval_mining.rs | Adds integration tests for interval mining and evm_setIntervalMining behavior. |
| crates/edr_provider/src/spec.rs | Tightens SyncProviderSpec bounds to ensure RPC request types are Send. |
| crates/edr_provider/src/requests/eth/mine.rs | Refactors interval mining config update to mutate ProviderData directly. |
| crates/edr_provider/src/provider.rs | Reworks Provider to send requests over channels to a background thread; adds enqueue API. |
| crates/edr_provider/src/lib.rs | Wires in the new backend module; removes old interval module. |
| crates/edr_provider/src/interval.rs | Removes the prior tokio-task-based IntervalMiner implementation. |
| crates/edr_provider/src/error.rs | Adds ProviderError::UnexpectedTermination and maps it to INTERNAL_ERROR. |
| crates/edr_provider/src/data.rs | Stores interval mining config in ProviderData and exposes getter/setter. |
| crates/edr_provider/src/config.rs | Derives Eq/PartialEq for IntervalConfig to support reschedule detection. |
| crates/edr_provider/src/backend.rs | New background-thread event loop handling requests and interval mining scheduling. |
| crates/edr_provider/Cargo.toml | Adds crossbeam-channel and edr_utils_sync dependencies. |
| crates/edr_napi/src/provider.rs | Changes handle_request to return a JS Promise resolved via provider enqueue callback; scenario file now shared via Arc. |
| crates/edr_napi/src/mock.rs | Updates mock provider to implement the new enqueue-based trait. |
| crates/edr_napi_core/src/provider.rs | Replaces blocking handle_request with enqueue-based API; refactors failed-deserialization handling. |
| crates/edr_generic/tests/integration/issues/issue_947.rs | Updates trait bounds to satisfy new provider constraints. |
| crates/edr_generic/tests/integration/helpers.rs | Updates helper trait bounds to satisfy new provider constraints. |
| Cargo.lock | Records new dependency additions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let result = data | ||
| .logger_mut() | ||
| .print_method_logs(&method_name, Some(&error)) | ||
| .map_err(ProviderError::Logger); |
| result, | ||
| call_trace_arenas: traces, | ||
| }) | ||
| response_receiver.recv().expect(BACKEND_THREAD_TERMINATED) |
| let (response_sender, response_receiver) = bounded(1); | ||
| self.request_sender | ||
| .send(new_request_fn(response_sender)) | ||
| .expect(BACKEND_THREAD_TERMINATED); | ||
|
|
||
| response_receiver.recv().expect(BACKEND_THREAD_TERMINATED) | ||
| } |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
🚀 Starting regression benchmark for |
|
✅ Regression benchmark passed for |
anaPerezGhiglia
left a comment
anaPerezGhiglia
left a comment
There was a problem hiding this comment.
Really nice! the design direction and the benchmark numbers speak for themselves 🙌
None of my comments is blocking: they're a mix of pre-existing validation holes this design escalates (proposed as a follow-up issue + fix), a hardening suggestion for the event loop's panic path and structure/naming suggestions.
Submitting as "comment" rather than approval only because several of these may turn into changes worth a second look. Happy to approve once the discussions settle!
| /// Configuration for interval mining, if enabled. The provider's background | ||
| /// thread owns the interval timer and reads this field to (re)schedule it. |
There was a problem hiding this comment.
I'm not fan of these type of Claude comments where we detail the internals of the consumer of this information.
| /// Sets the interval mining configuration. The provider's background thread | ||
| /// picks up the change after the current request and reschedules its timer | ||
| /// accordingly. Passing `None` disables interval mining. |
There was a problem hiding this comment.
same here. This method does not care about the provider's background thread. I feel these type of comments ends up breaking the encapsulation built on the code, making them super hard to maintain
| /// returning immediately without waiting for the request to be handled, as | ||
| /// the caller may be the JS main thread. |
There was a problem hiding this comment.
returning immediately might be too harsh. Actually the impl 20 lines below does not follow it.
| /// returning immediately without waiting for the request to be handled, as | |
| /// the caller may be the JS main thread. | |
| /// as callers (e.g. the JS main thread) rely on this method returning without | |
| /// waiting for the request to be handled. |
| let mut interval_timer = next_interval_timer(data.interval_config()); | ||
|
|
||
| loop { | ||
| crossbeam_channel::select_biased! { |
There was a problem hiding this comment.
By design, I agree with having a single background thread that works as an event loop with this prioritization: for any valid interval config, the timer is re-armed only after mining completes, so the request queue always keeps making progress.
Unfortunately, interval mining has some pre-existing validation holes that compromise the whole provider under this design, since the thread that mines is now the same one that serves requests (both reproduced on this branch):
Range { min: 0, max: 0 }is accepted on both entry points.generate_interval()returns 0 on every draw, so the timer is immediately ready on every re-arm andselect_biased!always picks mining: requests are never serviced and their JS promises never settle, while the loop hot-mines blocks. Onmain, the same config busy-mines but requests still progress through the mutex.Range { min > max }is rejected on the RPC path but not on the napi initial-config path;random_rangepanics on the empty range when the timer is armed. Onmainthat panic killed the detached mining task and the provider kept serving requests; here it kills the background thread that ownsProviderData, leaving the provider unusable.
Neither hole is introduced by this PR and both are edge cases, so I don't think this should block it. But since this design escalates them from "noisy/degraded" to "provider compromised", I'd address them as a separate issue + fix shortly after this lands. The fix is contained: normalize/reject at the two conversion points, mirroring the existing "zero implies disabled" scalar semantics. The event loop itself should need no changes.
| while let Ok(message) = request_receiver.try_recv() { | ||
| if let BackendRequest::Request { on_response, .. } = message { | ||
| on_response(Err(ProviderError::UnexpectedTermination)); | ||
| } | ||
| } |
There was a problem hiding this comment.
On a background-thread panic this drain loop is skipped (the thread unwinds out of it), so an already-queued request is dropped unsettled: its JS promise dangles forever, and the blocking handle_request panics on the caller's thread via .expect(BACKEND_THREAD_TERMINATED).
I reproduced this locally with an interval mining config of min > max (see my other comment). Claude suggested moving the drain into a drop guard, which I tested locally and works, but I haven't spent time investigating other alternatives.
| recv(cancellation_receiver) -> _ => break, | ||
| // Interval mining takes precedence over incoming requests. An overdue | ||
| // deadline yields a zero duration, so `after` is immediately ready. | ||
| recv(interval_timer) -> _ => { |
There was a problem hiding this comment.
I think we should reassess what valid values for the mining interval are: with a valid-but-very-small interval, request handling can get throttled because the event loop is "too busy" mining blocks. To users this would surface as EDR taking long to resolve requests.
| fn execute_request<ChainSpecT, TimerT>( | ||
| data: &mut ProviderData<ChainSpecT, TimerT>, | ||
| request: ProviderRequest<ChainSpecT>, | ||
| ) -> Result<ResponseWithCallTraces, ProviderErrorForChainSpec<ChainSpecT>> | ||
| where | ||
| ChainSpecT: SyncProviderSpec< | ||
| TimerT, | ||
| PooledTransaction: IsEip155, | ||
| SignedTransaction: Default | ||
| + TransactionMut | ||
| + TransactionType<Type: IsEip4844> | ||
| + TransactionValidation<ValidationError: PartialEq>, | ||
| >, | ||
| TimerT: Clone + TimeSinceEpoch, | ||
| { | ||
| match request { | ||
| ProviderRequest::Single(request) => execute_single_request(data, *request), | ||
| ProviderRequest::Batch(requests) => execute_batch_request(data, requests), | ||
| } | ||
| } | ||
|
|
||
| /// Executes a batch of JSON requests for an execution provider. | ||
| fn execute_batch_request<ChainSpecT, TimerT>( | ||
| data: &mut ProviderData<ChainSpecT, TimerT>, | ||
| request: Vec<MethodInvocation<ChainSpecT>>, | ||
| ) -> Result<ResponseWithCallTraces, ProviderErrorForChainSpec<ChainSpecT>> | ||
| where | ||
| ChainSpecT: SyncProviderSpec< | ||
| TimerT, | ||
| PooledTransaction: IsEip155, | ||
| SignedTransaction: Default | ||
| + TransactionMut | ||
| + TransactionType<Type: IsEip4844> | ||
| + TransactionValidation<ValidationError: PartialEq>, | ||
| >, | ||
| TimerT: Clone + TimeSinceEpoch, | ||
| { | ||
| let mut results = Vec::new(); | ||
| let mut traces = Vec::new(); | ||
|
|
||
| for req in request { | ||
| let response = execute_single_request(data, req)?; | ||
| results.push(response.result); | ||
| traces.extend(response.call_trace_arenas); | ||
| } | ||
|
|
||
| let result = serde_json::to_value(results).map_err(ProviderError::Serialization)?; | ||
| Ok(ResponseWithCallTraces { | ||
| result, | ||
| call_trace_arenas: traces, | ||
| }) | ||
| } | ||
|
|
||
| fn execute_single_request<ChainSpecT, TimerT>( |
There was a problem hiding this comment.
I would have expected this module to own a single responsibility: the event loop. execute_request / execute_batch_request / execute_single_request don't care where they run, they take &mut ProviderData and a request, nothing thread-related.
Concretely, I'd propose moving all three to the requests module: they route MethodInvocation to the handlers that already live there. Optionally, the big match in execute_single_request could be extracted into its own routing function, separating it from the method-logging wrapper around it.
That would leave this module with only the code that is genuinely about scheduling, and make the dispatch match testable as plain routing, with no logging concerns mixed in.
| // layout-computation recursion limit of 128. | ||
| #![recursion_limit = "256"] | ||
|
|
||
| mod backend; |
There was a problem hiding this comment.
nitpick: I'm not a fan of this name. I feel it's vague and it's not clear what it's responsible for. What do you think about event_loop?
That way we'd have event_loop::run and event_loop::Message instead of BackendRequest (BACKEND_THREAD_TERMINATED const should be renamed too to follow along).
| alloy-sol-types.workspace = true | ||
| anyhow = { workspace = true, optional = true } | ||
| auto_impl = { version = "1.2", default-features = false } | ||
| crossbeam-channel = "0.5" |
There was a problem hiding this comment.
there are already three workspace members using this dependency. Should we define it in [workspace.dependencies] like the other shared external deps?
| let (response_sender, response_receiver) = bounded(1); | ||
|
|
||
| for req in request { | ||
| let response = self.handle_single_request(data, req)?; | ||
| results.push(response.result); | ||
| traces.extend(response.call_trace_arenas); | ||
| } | ||
| self.enqueue_request( | ||
| request, | ||
| Box::new(move |response| { | ||
| // Ignore the error: the caller may have stopped waiting. | ||
| let _ = response_sender.send(response); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
I think handle_request, send_request_and_wait and enqueue_request are re-implementing pieces of each other, with diverging failure handling: send_request_and_wait panics via .expect(BACKEND_THREAD_TERMINATED) where enqueue_request gracefully settles the callback with UnexpectedTermination. Same failure, two behaviors, three lines apart.
What do you think about splitting this into two building blocks?
send_message(message) -> Result<(), BackendRequest>: the raw send, handing the message back on failure. Shared byenqueue_request(which needs the message back to recover its callback) andsend_request_and_wait.wait_for_reply(enqueue_fn) -> Result<ResponseT, ProviderError>: creates thebounded(1)reply channel, lets the closure dispatch a message embedding the sender, and maps a disconnectedrecvtoUnexpectedTermination. Shared byhandle_request(composed on top ofenqueue_request, so the blocking API keeps exercising the exact same path as the napi caller) andsend_request_and_wait.
With that, send_request_and_wait doesn't even need explicit error handling.
I tried this locally and the full edr_provider suite stays green, so happy to share the patch if useful.
GitHub Actions
Error
a178475 to
2996ae3
Compare
GitHub Actions
Inactive
Wodann
changed the title
GitHub Actions
Inactive
GitHub Actions
Inactive
Replaces the synchronisation mechanism of the JSON-RPC provider from mutexes and tokio blocking threads to channels and a dedicated OS thread. Requests and interval mining are serialised on the thread that owns the
ProviderData, without any locking.Design
The napi
handleRequestmethod deserialises the request on the JS thread (microseconds), enqueues it on the provider's channel, and returns a promise. The provider's background thread executes the request and settles the promise directly — aJsDeferredcan be settled from any thread. That makes a request cost two thread handoffs (JS → provider thread → JS), fewer thanmain, which paid for aspawn_blockinground trip on every request.Consequences:
Provider::handle_requestis reimplemented on top of the callback path and keeps its public API (used by tests).ProviderError::UnexpectedTermination, so JS promises never dangle.cast_response) now runs on the background thread; request deserialisation runs on the JS thread.Considered designs
The naive design — send over a channel, block a tokio blocking thread on the reply — regressed the provider benchmark ~15% on request-dense scenarios: four thread handoffs per request (+2.3 context switches, +4.8 futex syscalls, a fixed ~19 µs). Three fixes were implemented and benchmarked locally (rocketpool scenario, median of 5 runs,
perf statfor the counters):recv()(this PR, before fix)parking_lot::Mutex; thread only for interval miningThe mutex variant proved the regression was purely the per-request channel round trip, but reintroduces the locking this PR set out to remove. The oneshot variant fixes the regression but keeps the blocking-pool hop. Settling the promise from the provider thread removes that hop too — it turns out to be roughly half of the provider benchmark's wall time: the full local suite drops from 442 s (merge-base) to 220 s.
Benchmark results
JS scenario runner benchmark (CI, vs latest
main):HH3 regression benchmark (CI, vs latest
main):The scenario runner improves more than HH3 because it replays only the JSON-RPC request stream against the provider — it isolates exactly the slice this PR changes. HH3 wall-clock time also includes the test runner, the test suite's own JS, client libraries and process startup, so the removed per-request synchronisation cost is diluted; how much a suite improves tracks how much of its runtime is spent in provider calls.
Validation
mainin every scenario, locally and in CI.cargo test -p edr_provider --features test-utils: 247 tests pass, including the interval mining integration tests.perf statcontext-switch and futex counts (table above) confirm the thread handoffs are actually gone, not just faster.