Fix server IO->CPU dispatch so rpc_num_cpu_worker_threads > 1 works (#749) - #749
Closed
charles-typ wants to merge 1 commit into
Closed
Fix server IO->CPU dispatch so rpc_num_cpu_worker_threads > 1 works (#749)#749charles-typ wants to merge 1 commit into
charles-typ wants to merge 1 commit into
Conversation
|
@charles-typ has exported this pull request. If you are a Meta employee, you can view the originating Diff in D110506501. |
…acebookresearch#749) Summary: Setting `rpc_num_cpu_worker_threads > 1` made the server go completely idle (~2% CPU, requests never processed) during a benchmark, so the flag was unusable and the prod-like "IO -> CPU -> IO" dispatch pattern it is meant to exercise never actually ran. Root cause: the Thrift handlers are `async_eb_*`, so they must complete their `HandlerCallback` on the callback's own EventBase. `ucacheBenchOnRequestCommon` offloaded work to a `folly::CPUThreadPoolExecutor` (from `getCpuPool()`, only created when the flag is > 1) and then invoked the handler — which called `cb->result(...)` — directly on the CPU-pool thread. Those pool threads are not EventBase threads and are unknown to the per-EventBase handler map, so the reply was never delivered. In-flight requests never drained, clients stopped sending, and the server sat idle. With the flag == 1, `getCpuPool()` returns `nullptr`, everything runs inline on the IO thread, and it works — which is why the bug only showed up for > 1. Fix: split compute from delivery. The per-request `handler` now COMPUTES and returns the reply; `ucacheBenchOnRequestCommon` owns delivering it via `callback->result(...)`. On the CPU-pool path it captures the callback's EventBase up front, runs the compute on the pool, then bounces the completion back with `evb->runInEventBaseThread(...)` so the async_eb callback is completed on its own EventBase. The inline and fiber paths are behaviorally unchanged for the default `rpc_num_cpu_worker_threads == 1`. Verified: with the fix, `rpc_num_cpu_worker_threads=64` drives real traffic and the run completes cleanly ("All clients finished benchmark"), where before it hung the server at ~2% CPU. Note: the dispatch path trades throughput for the extra IO->CPU->IO context switches (as intended, to match production's tao:slow scheduling); it is a fidelity knob, not a throughput/utilization lever. Reviewed By: excelle08 Differential Revision: D110506501
charles-typ
force-pushed
the
export-D110506501-to-v2-beta
branch
from
July 6, 2026 18:02
2a6bd15 to
81b296e
Compare
meta-codesync Bot
pushed a commit
that referenced
this pull request
Jul 7, 2026
…749) Summary: Pull Request resolved: #749 Setting `rpc_num_cpu_worker_threads > 1` made the server go completely idle (~2% CPU, requests never processed) during a benchmark, so the flag was unusable and the prod-like "IO -> CPU -> IO" dispatch pattern it is meant to exercise never actually ran. Root cause: the Thrift handlers are `async_eb_*`, so they must complete their `HandlerCallback` on the callback's own EventBase. `ucacheBenchOnRequestCommon` offloaded work to a `folly::CPUThreadPoolExecutor` (from `getCpuPool()`, only created when the flag is > 1) and then invoked the handler — which called `cb->result(...)` — directly on the CPU-pool thread. Those pool threads are not EventBase threads and are unknown to the per-EventBase handler map, so the reply was never delivered. In-flight requests never drained, clients stopped sending, and the server sat idle. With the flag == 1, `getCpuPool()` returns `nullptr`, everything runs inline on the IO thread, and it works — which is why the bug only showed up for > 1. Fix: split compute from delivery. The per-request `handler` now COMPUTES and returns the reply; `ucacheBenchOnRequestCommon` owns delivering it via `callback->result(...)`. On the CPU-pool path it captures the callback's EventBase up front, runs the compute on the pool, then bounces the completion back with `evb->runInEventBaseThread(...)` so the async_eb callback is completed on its own EventBase. The inline and fiber paths are behaviorally unchanged for the default `rpc_num_cpu_worker_threads == 1`. Verified: with the fix, `rpc_num_cpu_worker_threads=64` drives real traffic and the run completes cleanly ("All clients finished benchmark"), where before it hung the server at ~2% CPU. Note: the dispatch path trades throughput for the extra IO->CPU->IO context switches (as intended, to match production's tao:slow scheduling); it is a fidelity knob, not a throughput/utilization lever. Reviewed By: excelle08 Differential Revision: D110506501 fbshipit-source-id: afd5931e5f78d85b7ead8bf87c1fda97a5bca37f
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary:
Setting
rpc_num_cpu_worker_threads > 1made the server go completely idle (~2% CPU, requests never processed) during a benchmark, so the flag was unusable and the prod-like "IO -> CPU -> IO" dispatch pattern it is meant to exercise never actually ran.Root cause: the Thrift handlers are
async_eb_*, so they must complete theirHandlerCallbackon the callback's own EventBase.ucacheBenchOnRequestCommonoffloaded work to afolly::CPUThreadPoolExecutor(fromgetCpuPool(), only created when the flag is > 1) and then invoked the handler — which calledcb->result(...)— directly on the CPU-pool thread. Those pool threads are not EventBase threads and are unknown to the per-EventBase handler map, so the reply was never delivered. In-flight requests never drained, clients stopped sending, and the server sat idle. With the flag == 1,getCpuPool()returnsnullptr, everything runs inline on the IO thread, and it works — which is why the bug only showed up for > 1.Fix: split compute from delivery. The per-request
handlernow COMPUTES and returns the reply;ucacheBenchOnRequestCommonowns delivering it viacallback->result(...). On the CPU-pool path it captures the callback's EventBase up front, runs the compute on the pool, then bounces the completion back withevb->runInEventBaseThread(...)so the async_eb callback is completed on its own EventBase. The inline and fiber paths are behaviorally unchanged for the defaultrpc_num_cpu_worker_threads == 1.Verified: with the fix,
rpc_num_cpu_worker_threads=64drives real traffic and the run completes cleanly ("All clients finished benchmark"), where before it hung the server at ~2% CPU. Note: the dispatch path trades throughput for the extra IO->CPU->IO context switches (as intended, to match production's tao:slow scheduling); it is a fidelity knob, not a throughput/utilization lever.Reviewed By: excelle08
Differential Revision: D110506501