Uh oh!
There was an error while loading. Please reload this page.
perf: Reduce Box and Arc allocation churn during tree rewriting - #21749
Merged
Dandandan merged 3 commits intoApr 21, 2026
Conversation
neilconway
commented
Apr 20, 2026
ContributorAuthor
run benchmark sql_planner |
adriangbot
commented
Apr 20, 2026
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-planner-box-arc-take (561de69) to a311d14 (merge-base) diff File an issue against this benchmark runner |
Box and Arc allocation churn in the plannerBox and Arc allocation churn in the plannerBox and Arc allocation churn in the plannerBox and Arc allocation churn in the optimizerBox and Arc allocation churn in the optimizerBox and Arc allocation churn in logical optimizerneilconway
marked this pull request as ready for review
April 20, 2026 20:43
Box and Arc allocation churn in logical optimizerBox and Arc allocation churn during tree rewritingDandandan
commented
Apr 21, 2026
Contributor
run benchmark tpch |
Dandandan
commented
Apr 21, 2026
Contributor
run benchmark sql_planner |
Dandandan
commented
Apr 21, 2026
Contributor
@neilconway which tool are you using for profiling, I wonder? |
adriangbot
commented
Apr 21, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-planner-box-arc-take (71924ef) to a311d14 (merge-base) diff using: tpch File an issue against this benchmark runner |
adriangbot
commented
Apr 21, 2026
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-planner-box-arc-take (71924ef) to a311d14 (merge-base) diff File an issue against this benchmark runner |
| fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>( | ||
| self, | ||
| mut self, | ||
| f: F, |
Contributor
There was a problem hiding this comment.
Optimizing this API is great, but I wonder two things:
- Do we call this API too much in some passes? i.e. should we make some optimizer pass more efficient by avoiding / reducing the need to call this API at all in some places?
- Are we using the "wrong" abstractions that inherently lead to inneficient code? (I.e. lot's of closures, branches, nesting -> hard to optimize for compiler)
Dandandan
commented
Apr 21, 2026
Contributor
run benchmark clickbench_partitioned |
adriangbot
commented
Apr 21, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-planner-box-arc-take (71924ef) to a311d14 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
adriangbot
commented
Apr 21, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
martin-g
approved these changes
Apr 21, 2026
xudong963
approved these changes
Apr 21, 2026
| }) | ||
| .ok() | ||
| })); | ||
| assert!(result.is_err()); |
Member
There was a problem hiding this comment.
It doesn't verify the actual panic-safety invariant — that dropping an unwound Box doesn't double-free or access garbage
ContributorAuthor
There was a problem hiding this comment.
True, although I'm not sure there's a good way to validate that in a standard unit test (unless we run under miri or similar). wdyt?
adriangbot
commented
Apr 21, 2026
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Uh oh!
There was an error while loading. Please reload this page.
adriangbot
commented
Apr 21, 2026
Benchmark for this request hit the 7200s job deadline before finishing. Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
neilconway
commented
Apr 21, 2026
ContributorAuthor
I've been using |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Profiling the planner suggests that a surprising amount of time was being spent doing tree rewriting in the logical optimizer. One culprit is
TreeNodeContainer::map_elements()forBox<C>andArc<C>, which do the following:Cvalue from theBox/ArcBox/Arc, respectivelyThis allocates a fresh
BoxorArcfor every node visited while walking an expression or logical plan, even if the tree rewrite we're doing didn't modify the expression/plan node.Instead, we can reuse the current
Box<C>orArc<C>: usestd::mem::take()to swap the inner value withC::default(), pass the inner value to the closure, and put the result back in the original container. Swapping the inner value withC::default()means the container always has a valid value, which is important if the closure panics.For
Arc<C>, we need to useArc::make_mut(), which only clones if theArcis not unique.This reduces the bytes allocated to plan TPC-H Q13 by ~22% (988 kB -> 765 kB), and reduces allocated blocks by 8.5% (210k -> 192k).
What changes are included in this PR?
Box<C>::map_elements()andArc<C>::map_elements()as described abovemap_children()forExpr::Aliasto usemap_elements(), rather than invokingf(*expr)directly; this ensures that it can take advantage of this optimizationLogicalPlan::default()use a sharedDFSchema, rather than allocating a freshDFSchemafor every call. Becausedefault()is not in the hot path for tree rewriting, it is important that it is cheapmap_elements()behaviorAre these changes tested?
Yes, plus new unit tests added.
Are there any user-facing changes?
Yes:
TreeNodeContainerimpls forBox<C>andArc<C>now requireC: Default. This is a breaking API change for third-party code that implementsTreeNodeContainerfor a custom type. The fix is usually straightforward.