Dandandan opened a new pull request, #24330:
URL: https://github.com/apache/datafusion/pull/24330
## Which issue does this PR close?
Fourth and last crate in the family from #24325 (`datafusion-catalog`),
#24326
(`datafusion-session`) and #24329 (`datafusion` core). Independent of all
three —
different crates, so they can merge in any order.
## Rationale for this change
`datafusion-catalog-listing` is 3,013 lines of source but spends **20.5s**
in the
frontend during a cold `cargo build -p datafusion` (`cargo build
--timings`), and
it sits on the critical path between `datafusion-catalog` and `datafusion`
core.
`-Zself-profile` puts 58% of the crate's compile time in
`evaluate_obligation`,
and grouping those goals by the `Self` type in their `ParamEnv` shows all of
it
in one impl:
| `Self` in `ParamEnv` | time | goals |
|---|---|---|
| `ListingTable` | 3.16s | 6,967 |
| *(empty `ParamEnv`)* | 0.04s | 8,443 |
Note the second row — the same kind of goals cost ~5µs each with an empty
`ParamEnv` against ~450µs here.
The cause is the one from the earlier PRs: `#[async_trait]` gives each
`async fn`
a `where 'life0: 'async_trait, ..` clause, which makes the method's
`ParamEnv`
non-empty, and rustc only serves auto-trait obligations from its **global**
evaluation cache when the `ParamEnv` is empty. So the `Send`/`Sync` proof for
everything the future captures is redone per method.
All three async methods in this impl reach `Expr`:
- `scan` takes `&[Expr]`
- `scan_with_args` takes `ScanArgs<'a>`, which holds `&[Expr]`
- `insert_into` keeps `self.options` (`Vec<Vec<SortExpr>>`) live across an
await
so each one pays for a walk of the whole `Expr`/`LogicalPlan` graph.
## What changes are included in this PR?
Each method is now the hand-written desugaring of `async fn` and only
forwards;
the coroutine is built in a shim with no where-clauses, so its proofs land
in the
global cache. Bodies are moved verbatim into inherent fns and all three stay
`async`, so nothing is evaluated any earlier than before —
`Box::pin(self.m_inner(..))` polls nothing.
Following the review on #24326, I measured each method's marginal
contribution
first, by reverting one at a time (together with its helpers) from the
all-converted state:
| state | crate build | `evaluate_obligation` |
|---|---|---|
| all three converted | 0.931s | 34.9ms |
| revert `insert_into` | 1.866s | 1.01s |
| revert `scan` | 1.943s | 1.05s |
| revert `scan_with_args` | 2.118s | 1.17s |
| none converted (base) | 4.201s | 3.25s |
Unlike #24326 — where two of the seven bodies I first converted turned out to
gain nothing — all three pull their weight here. Converting all of them
leaves no
coroutine in the impl at all, so the graph is never walked in a non-empty
`ParamEnv`, which is why the total drops by two orders of magnitude rather
than
by a third.
## Are these changes tested?
- `cargo test -p datafusion-catalog-listing` — 18 + 7 passed
- `cargo test -p datafusion --lib` — 442 passed
- `cargo check -p datafusion --all-targets` — clean (`ListingTable` is used
heavily by core's integration tests and benches)
- `cargo clippy -p datafusion-catalog-listing --all-targets` — clean
- `cargo fmt --check` — clean
The compiler checks each rewritten signature against the trait declaration,
and
every body is moved verbatim.
Interleaved A/B of `cargo rustc -p datafusion-catalog-listing --lib`,
alternating
3 times so machine drift cancels out:
```
base: 4.257s 4.189s 4.134s
fix: 0.984s 0.935s 0.980s
```
`evaluate_obligation` drops from 3.25s to 34.7ms.
## Are there any user-facing changes?
No. No public signature changes — after macro expansion these methods have
the
same signatures as before.
### Follow-up
With this, the four crates that made up the serial tail of a cold build are
done.
The general fix remains available and would cover downstream implementors
too:
drop `#[async_trait]` from these traits in favour of an explicit `BoxFuture`
return with a single lifetime and no where-clauses, so that *every* impl is
cheap
without hand-desugaring. 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]
