Skip to content

fix(volo-thrift): create spans before constructing futures - #674

Open
shenyj3 wants to merge 1 commit into
mainfrom
fix/thrift-span-before-future
Open

shenyj3 wants to merge 1 commit into
mainfrom
fix/thrift-span-before-future

Conversation

@shenyj3

@shenyj3 shenyj3 commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

The ping-pong server can perform avoidable large memory copies while constructing the future for each decoded request. This becomes expensive when the concrete service/middleware stack produces a large future and the span hook remains a potentially unwinding call after optimization.

The relevant code is in volo-thrift/src/transport/pingpong/server.rs:

// Before, simplified: tracing_cx was obtained through an unsafe transmute.
let result = async {
    // Handle the request, await the service, and encode the response.
}
.instrument(span_provider.on_serve(tracing_cx))
.await;

How the copies arise

  1. The method-call receiver is evaluated before its argument. Therefore, the async { ... } future is constructed before on_serve(...) is called. Its captured values are already part of that future, even though its body has not been polled yet. See the Rust Reference on operand evaluation order and async blocks.
  2. The request future needs storage for the states it can enter later, including the service future held across service.call(...).await. A large nested service future can consequently make the enclosing request future large, even though those later states are not active during construction.
  3. If on_serve unwinds, the already-constructed temporary request future must be dropped correctly. The compiler must preserve a cleanup path for that temporary across the hook call. When that cleanup keeps the aggregate opaque to optimization, it can prevent the compiler from splitting the initial state into individual fields and eliminating unused storage from temporary moves.
  4. After the hook returns normally, wrapping the future in Instrumented and moving it into the enclosing async state machine can then retain copies of the full aggregate. Such Rust ownership moves can lower to memcpy/memmove; they do not require an explicit .clone() in the source.

A panic does not need to occur for this cost to be paid. The possible unwind path can affect optimization of the normal request path. Correct unwinding also does not inherently require full-size copies: whether they survive depends on inlining, concrete types, and compiler optimization.

This setup runs inside the connection future's poll, which can make the copies appear under a polling frame in a CPU profile. The relevant operation is constructing a new per-request future; this does not imply that every subsequent poll relocates an already pinned future.

Why the effect varies between services

The future layout depends on the concrete service and middleware types. The span provider matters as well: DefaultProvider uses the trait's Span::none() implementation, which may inline away, while another provider may retain a call and its unwind edge. Compiler versions, optimization settings, and surrounding code can also change which aggregate moves survive. A large size_of::<Future>() alone therefore does not establish either the number of copies or their CPU cost.

Solution

Evaluate the hook before constructing the future:

let serve_span = span_provider.on_serve(&cx);
let result = async {
    // Handle the request, await the service, and encode the response.
}
.instrument(serve_span)
.await;

When on_serve runs, the large temporary request future now does not exist yet. An unwind from that call cleans up the existing locals without needing to drop that future. This removes the particular cleanup dependency that can keep the full temporary aggregate materialized, allowing the compiler to optimize construction around the fields that are actually initialized.

The future still needs storage for its later suspended states. The intended improvement is fewer bytes moved during construction; a reduction in the future's type size is not required.

Apply the same ordering to on_encode, creating encode_span before constructing the encoding async block. Both hooks can then borrow ServerContext directly before the corresponding future captures it, so the lifetime-extending unsafe transmute can be removed.

The change adds no boxing or new runtime allocation. on_serve still runs after decoding and before request processing; on_encode still runs after the response metadata is prepared and before encoding. The request and encoding futures retain their respective Instrumented wrappers.

Validation

Four tests exercise the actual ping-pong serve function with a recording span provider and a tracing subscriber:

  • Normal request: verify hook order, decoded context, and the active request/response spans before and after asynchronous suspension.
  • Oneway request: verify request hooks run without creating an encoding span.
  • Service error: verify the exception response is encoded under the response span.
  • Encoding failure: verify leave_encode runs and the existing early-return behavior is preserved.

Local checks:

  • cargo test -p volo-thrift --locked --offline --lib: 37 passed.
  • cargo test -p volo-thrift --locked --offline --all-features: 51 passed; 6 existing doctests ignored.
  • cargo clippy --locked --offline -p volo-thrift --all-features --all-targets -- -D warnings: passed.
  • cargo +nightly fmt -p volo-thrift -- --check: passed.

These tests cover tracing and request-processing behavior. They do not enforce a particular LLVM optimization or establish an end-to-end CPU improvement. Performance validation for a concrete service should compare optimized code generation around request-future construction with identical types and build settings, then measure the resulting runtime under the same workload.

@codecov

codecov Bot commented Sep 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.91%. Comparing base (90aa52b) to head (f5465e8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #674      +/-   ##
==========================================
+ Coverage   56.14%   56.91%   +0.76%     
==========================================
  Files         166      166              
  Lines       22517    22561      +44     
==========================================
+ Hits        12642    12840     +198     
+ Misses       9875     9721     -154     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants