Skip to content

perf: desugar remaining #[async_trait] stubs capturing Expr/LogicalPlan - #24370

Closed
alamb wants to merge 1 commit into
apache:mainfrom
alamb:desugar-remaining-async-trait
Closed

perf: desugar remaining #[async_trait] stubs capturing Expr/LogicalPlan#24370
alamb wants to merge 1 commit into
apache:mainfrom
alamb:desugar-remaining-async-trait

Conversation

@alamb

@alamb alamb commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Applies the same rewrite as the four PRs above to the remaining #[async_trait] methods on the serial build path whose futures capture Expr/LogicalPlan-reaching types (so their Send/Sync proofs are re-proved per method in a non-empty ParamEnv):

  • UnsupportedQueryPlanner::create_physical_plan (datafusion-session) — captures &LogicalPlan
  • ExtensionPlanner::plan_table_scan default body (datafusion-session) — captures &TableScan, which holds Vec<Expr>
  • EmptyTable::scan (datafusion-catalog) — captures &[Expr]
  • TestTableProvider::scan (datafusion core; test_util compiles into the lib unconditionally) — captures &[Expr]

What changes are included in this PR?

Each method becomes the hand-written desugaring of what #[async_trait] generates (verified against -Zunpretty=expanded output), with no coroutine capturing the arguments:

  • The stubs (create_physical_plan, plan_table_scan, TestTableProvider::scan) return ready(..) or a captureless async block directly.
  • EmptyTable::scan has no .await, so its body moves verbatim to a plain inherent fn wrapped in ready(..) (same as TestTableFactory::create_inner in perf(core): cut datafusion core compile time ~10x #24329).
  • plan_table_scan keeps its Self: Sync + 'async_trait bound so the public trait signature is unchanged.

Each converted method carries the explanatory comment from #24362.

Are these changes tested?

Functionally: cargo fmt --check, cargo clippy -p datafusion-catalog -p datafusion-session --all-targets, cargo clippy -p datafusion --lib, cargo test -p datafusion-catalog -p datafusion-session, and cargo test -p datafusion --lib all pass. The compiler checks each rewritten signature against its trait declaration.

Compile-time impact is not yet measured (my machine is too noisy); this PR is a draft until the numbers below are collected.

Compile-time measurements to run (on a quiet machine)

Interleaved A/B of each affected crate's unit, alternating main and this branch at least 3 times each so machine drift cancels out (the methodology from #24325#24330):

# once per checkout: warm the dependency graph
cargo build -p datafusion

# one round (repeat >=3x, alternating branches):
cargo clean -p datafusion-session -p datafusion-catalog -p datafusion
time cargo rustc -p datafusion-session --lib
time cargo rustc -p datafusion-catalog --lib
time cargo rustc -p datafusion --lib

Expected: a drop in datafusion-session (which had ~0.65s of evaluate_obligation remaining after #24326 in the standalone build, several times more in the feature-unified build) and a smaller drop in datafusion-catalog and core. EmptyTable/TestTableProvider are unproven — they did not appear in the per-impl profiles of #24325/#24329 — so it is worth confirming their marginal contribution before merging, e.g. by reverting one file at a time.

To attribute the change to the trait solver rather than noise:

# cargo install --git https://github.com/rust-lang/measureme summarize
RUSTFLAGS="-Zself-profile=/tmp/self-profile" cargo +nightly rustc -p datafusion-session --lib
summarize summarize $(ls -t /tmp/self-profile/*.mm_profdata | head -1) | grep -E "evaluate_obligation|Total"

Are there any user-facing changes?

No. After macro expansion the method signatures are identical to before, including plan_table_scan's Self: Sync bound.

Applies the rewrite from apache#24325/apache#24326/apache#24329/apache#24330 to the last live
methods on the serial build path whose futures capture Expr- or
LogicalPlan-reaching types: UnsupportedQueryPlanner::create_physical_plan
and ExtensionPlanner::plan_table_scan (datafusion-session),
EmptyTable::scan (datafusion-catalog), and TestTableProvider::scan
(datafusion core). No public signature changes after macro expansion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added core Core DataFusion crate catalog Related to the catalog crate labels Aug 14, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 28.07018% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.18%. Comparing base (571477c) to head (d868efb).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/session/src/planner.rs 0.00% 28 Missing ⚠️
datafusion/core/src/test_util/mod.rs 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24370      +/-   ##
==========================================
- Coverage   81.19%   81.18%   -0.02%     
==========================================
  Files        1110     1110              
  Lines      388616   388677      +61     
  Branches   388616   388677      +61     
==========================================
  Hits       315534   315534              
- Misses      54500    54555      +55     
- Partials    18582    18588       +6     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alamb

alamb commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

I had codex run some tests on a gcp machine and this PR doesn't improve compialtion speed

Details below:

Compared PR head d868efb against its base 571477c, using 5 interleaved rounds. A full cargo clean and warm build were performed for each checkout; affected package artifacts were cleaned before each timed round.

Crate Main times (s) PR times (s) Main mean (s) PR mean (s)
datafusion-session 36.56, 1.44, 1.44, 1.43, 1.46 36.35, 1.46, 1.46, 1.46, 1.46 8.466 8.438
datafusion-catalog 36.49, 3.47, 3.48, 3.58, 3.47 36.53, 3.50, 3.51, 3.59, 3.52 10.098 10.130
datafusion 14.51, 14.43, 14.46, 14.41, 14.45 14.56, 14.50, 14.44, 14.44, 14.49 14.452 14.486

The first session/catalog measurements are cold-build outliers. Excluding round 1, the PR was approximately 1.2% slower for session, 0.9% slower for catalog, and 0.2% slower for core—within normal timing noise on this machine.

Exact benchmark commands
# once per checkout
cargo clean
cargo build -p datafusion --quiet

# each round, before the timed commands
cargo clean -p datafusion-session -p datafusion-catalog -p datafusion --quiet

# timed commands
/usr/bin/time -f "RESULT package=datafusion-session seconds=%e" cargo rustc -p datafusion-session --lib --quiet
/usr/bin/time -f "RESULT package=datafusion-catalog seconds=%e" cargo rustc -p datafusion-catalog --lib --quiet
/usr/bin/time -f "RESULT package=datafusion seconds=%e" cargo rustc -p datafusion --lib --quiet

@alamb alamb closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

catalog Related to the catalog crate core Core DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants