perf: Avoid cloning EquivalenceProperties in ordering satisfaction checks - #24800
perf: Avoid cloning EquivalenceProperties in ordering satisfaction checks#24800jayzhan211 wants to merge 4 commits into
Conversation
…ed keys in requirements
…EquivalenceProperties
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24800 +/- ##
==========================================
- Coverage 81.63% 81.62% -0.01%
==========================================
Files 1123 1123
Lines 409673 409686 +13
Branches 409673 409686 +13
==========================================
+ Hits 334417 334422 +5
- Misses 55625 55631 +6
- Partials 19631 19633 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The lazy clone approach looks good to me, and I like that it also avoids updating the temporary state after the final key. I don't see any blocking issues.
I left one small suggestion about the Criterion benchmarks and what work we want them to measure.
| let props = properties(n_classes); | ||
|
|
||
| // A single sort key: the most common shape by far. | ||
| group.bench_with_input( |
There was a problem hiding this comment.
Nice to have benchmark coverage for this. One thought: if the goal is to isolate the cost of the satisfaction checks themselves, could we pre-build the ordering/requirement inputs and use iter_batched so the per-iteration setup stays outside the timed routine? Right now the closures also create Columns and PhysicalSortExprs, and the prefix benchmark builds the LexOrdering, so those costs are included in the measurements. If the intention is to measure the end-to-end caller cost instead, keeping the setup here makes sense, but it would be helpful to document that scope. We would still want any cloning that is part of the operation under test to remain inside the timed iteration.
There was a problem hiding this comment.
Good point — the setup was measurable, not incidental. asc(i) does a format! plus an Arc allocation per key per iteration, which was a real fraction of a ~400 ns measurement.
Switched to iter_batched: the sort exprs, requirements and the LexOrdering are now built once up front, and since each check takes its input by value, the untimed setup step hands each iteration a fresh clone. Only the call is timed. Also swapped bench_with_input for bench_function, as the parameter was only being used as a label.
Cloning done inside the check stays inside the timed iteration — that is the thing this PR is about, so it has to be measured.
It moved the numbers in the direction you'd expect. At 8 equivalence classes, 1 key:
| before | after | change | |
|---|---|---|---|
ordering_satisfy |
2.73 µs | 0.36 µs | −86.6% (was −84.9%) |
ordering_satisfy_requirement |
2.90 µs | 0.30 µs | −89.2% (was −86.3%) |
The old numbers were understating the improvement, since the constant setup cost sat in both columns.
I also documented the scope in the module header rather than leaving it implicit:
//! # Scope
//!
//! These measure the satisfaction check itself, not the cost of assembling its
//! arguments. The sort expressions, requirements and orderings are built once,
//! up front. Because the checks take their input by value, each iteration gets a
//! fresh copy from the untimed setup step of `iter_batched`; only the call is
//! timed. Any copying the check does internally is part of what is measured.
Metrics in the PR description updated to match.
…batched iterations
|
Thanks @kosiew ! |
Which issue does this PR close?
Rationale for this change
Physical planning asks "is this ordering already satisfied?" constantly — sort
removal,
EnforceSorting,EnforceDistribution, and the requirement checks forwindows, joins and aggregates all call into
EquivalenceProperties::ordering_satisfy,ordering_satisfy_requirementandextract_common_sort_prefix.Each of those calls deep-clones the entire
EquivalenceProperties— everyequivalence class, every equivalent ordering, and the normalized ordering cache —
before doing anything else, even when it never modifies the copy.
The clone exists for a real reason: as the check walks a multi-key ordering left
to right, it registers each satisfied key as a constant so the next key is
evaluated within that key's tie group. That mutates state, so it needs its own
copy. But two cases pay for it and get nothing back:
for, so the whole clone is wasted. This is the most common shape of these calls.
final key is verified, the code still calls
add_satisfied_key_constants,which rebuilds the ordering cache and re-runs ordering discovery — and then the
object is dropped.
What changes are included in this PR?
Two changes in
EquivalenceProperties, toordering_satisfy_requirementandcommon_sort_prefix_length(the latter backsordering_satisfy,extract_common_sort_prefixandreorder):selfand clonesonly when it actually needs to register a constant. Single-key checks never
clone at all.
Plus a new criterion benchmark,
equivalence_properties, covering these entrypoints.
This only changes when the copy is made — the results of these functions are
unchanged.
Metrics
Apple M4 Pro, rustc 1.97.0, criterion. All changes significant at p = 0.00.
Properties under test: 3 equivalent orderings (
[c0,c1,c2,c3],[c4,c5],[c6])and a varying number of equivalence classes.
At 8 equivalence classes:
ordering_satisfy— 1 keyordering_satisfy— 1 key, unsatisfiedordering_satisfy_requirement— 1 keyordering_satisfy_requirement— 4 keysordering_satisfy— 4 keysextract_common_sort_prefix— 4 keysHow it scales (
ordering_satisfy, 1 key):Reading the tables: for an N-key check the work goes from
1 clone + N registrationsto(N > 1 ? 1 : 0) clones + (N − 1) registrations.column is flat at ~0.41 µs regardless of how many equivalence classes exist —
with the clone gone, the check no longer scales with the size of the
equivalence group at all. The "before" column does, which is why the win grows
from −82% to −91%.
registration rebuilds the ordering cache and re-runs ordering discovery, that
single saved call is worth 9–14% here, rising to −37.8% for
4_keysat 32classes.
Reproducing
The benchmark is included in this PR, so reverting just the one source file gives
you the baseline:
The second run prints criterion's own
change: [...] (p = ...)line perbenchmark.
Are these changes tested?
No new correctness tests: this does not change what any of these functions
return, so existing coverage is the right check. Covered by the
equivalenceunit tests in
datafusion/physical-exprand, for plan-shape regressions, bysqllogictest — these functions decide whether a
SortExeccan be removed, so abehavior change would surface as a diff in an
EXPLAINplan.Full workspace suite
(
--features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption):10,981 passed, 0 failed, and all 505 sqllogictest files pass.
./dev/rust_lint.shis clean.
Are there any user-facing changes?
No. No public API or behavior changes — planning is just faster.