Dandandan opened a new pull request, #24329:
URL: https://github.com/apache/datafusion/pull/24329
## Which issue does this PR close?
Third and largest instance of the problem from #24325 (`datafusion-catalog`)
and
#24326 (`datafusion-session`). Independent of both — different crates, so
they
can merge in any order.
## Rationale for this change
`datafusion` core is the last unit of a cold `cargo build -p datafusion` and
compiles alone, so its cost lands directly on the build's wall clock. **76%
of
its compile time is the trait solver**: `-Zself-profile` reports 75.0s of
`evaluate_obligation` out of 98.6s total, essentially all of it proving
`Send`/`Sync`.
`#[async_trait]` gives each `async fn` a `where 'life0: 'async_trait, ..`
clause. rustc only serves auto-trait obligations from its **global**
evaluation
cache when the `ParamEnv` is empty, so the `Send`/`Sync` proof for everything
the returned future captures is redone per method. In this crate the captured
sets include `SessionState`, `&LogicalPlan` and `ListingTableConfig`, each of
which reaches a large fraction of the logical-plan type graph.
Grouping the goals by the `Self` type in their `ParamEnv` shows how
concentrated this is — 14 impls, top 10 = 78% of the total:
| impl | trait solving | | impl | trait solving |
|---|---|---|---|---|
| `ParquetReadOptions` | 7.73s | | `DynamicListTableFactory` | 5.18s |
| `JsonReadOptions` | 7.54s | | `ListingTableFactory` | 4.56s |
| `DefaultPhysicalPlanner` | 7.07s | | `TestTableFactory` | 4.47s |
| `CsvReadOptions` | 6.59s | | `ListingTableConfig` | 4.30s |
| `DataFrameTableProvider` | 5.68s | | `DefaultQueryPlanner` | 4.28s |
| *(trait default bodies)* | 5.37s | | `DefaultTableFactory` | 4.15s |
| | | | `SessionState` | 4.11s |
| | | | `ArrowReadOptions` | 3.72s |
For contrast, in the same compile 31,818 goals with an **empty** `ParamEnv`
cost
0.19s in total — 6µs each, against ~1.3ms for the same kind of goal under
`async_trait`'s bounds. Every impl in the table above was individually
attributed 3.7–7.7s, so none of these conversions is a no-op (the lesson from
#24326, where some were).
## What changes are included in this PR?
Each of those methods is now the hand-written desugaring of `async fn`, which
only forwards; the coroutine is built in a shim with no where-clauses, so its
auto-trait obligations are proved in an empty `ParamEnv` and land in the
global
cache. Method bodies are moved verbatim into inherent fns.
The `ReadOptions` family (25.6s across four impls, plus the 5.37s default
body)
collapses to a **single** proof: all five impls already delegated to the
`_get_resolved_schema` default body, which now hands the coroutine to a free
`infer_schema_boxed`. Because that helper is a plain function with no
generics
and no where-clauses, its proof is cached once and shared by every impl.
Two things worth noting for review:
- `DefaultPhysicalPlanner::create_initial_plan` already used exactly this
shape
(`-> BoxFuture<'a, _>` plus `Box::pin(async move ..)`) — there for
recursion
rather than for compile time. The idiom is not new to this codebase.
- One body became eager: `TestTableFactory::create_inner` has no `.await`,
so it
is a plain fn wrapped in `ready(..)`. It builds a `TestTableProvider` and
has
no side effects. Everything else that awaits stays lazy —
`Box::pin(self.m_inner(..))` polls nothing.
## Are these changes tested?
- `cargo test -p datafusion --lib` — 442 passed
- `cargo check -p datafusion --all-targets` — clean (this covers core's own
integration tests and benches, which are heavy users of these APIs)
- `cargo clippy -p datafusion --lib` — clean
- `cargo fmt --check` — clean
The compiler checks each rewritten signature against its trait declaration,
and
every body is moved verbatim.
Interleaved A/B of `cargo rustc -p datafusion --lib`, alternating so machine
drift cancels out:
```
base: 74.4s 69.9s
fix: 16.6s 15.5s
```
`evaluate_obligation` drops from **75.0s to 9.70s**, and its goal count from
57,901 to 37,757.
A third pair ran while a concurrent build saturated the machine (base 219.8s,
fix 32.5s) and is excluded from the table; its ratio was consistent with the
other two.
## Are there any user-facing changes?
No. No public signature changes — after macro expansion these methods have
the
same signatures as before.
### Remaining
`datafusion-catalog-listing` is the last crate in this family: 3.2s of
`evaluate_obligation` (58% of the crate), all 3.16s of it in the single
`impl TableProvider for ListingTable`.
Beyond that, the general fix would be to drop `#[async_trait]` from these
traits
in favour of an explicit `BoxFuture` return with a single lifetime and no
where-clauses, which would make *every* impl cheap, including in downstream
crates — but that is a breaking change to public traits, so it is out of
scope
here.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
