Cache PlanProperties, add fast-path for with_new_children
#19792
Conversation
with_new_childrenwith_new_children
72ff575 to
796f731
Compare
|
Also added a typical analytical query plan re-usage benchmark. On the $ cargo bench --profile=release-nonlto --bench plan_reuse |
796f731 to
5601c4f
Compare
|
I filed a ticket to track this idea |
|
run benchmark sql_planner |
|
🤖 |
Could you move the plan_reuse benchmark into its own PR (as I think it is valuable both for this PR and others, and it makes it easier to automatically compare performance) |
|
Benchmark script failed with exit code 101. Last 10 lines of output: Click to expand |
Done in #19806 |
5601c4f to
99cf634
Compare
99cf634 to
b81dd66
Compare
alamb
left a comment
There was a problem hiding this comment.
Thanks @askalt -- this is quite clever and I think it looks very promising
I also think we may be able to potentially make with_new_children even faster by checking the children as well -- and if they are the same there is no reason to recompute everything either.
However, this likely won't help your usecase as the children will likely change (their states need to be reset) 🤔
b81dd66 to
741b085
Compare
|
This PR seems to have accumulated some conflicts |
|
I plan to run the newly introduced benchmark in #19806 I suspect we'll see quite a nice improvement |
741b085 to
21b8a3f
Compare
|
run benchmark reset_plan_states |
|
🤖 |
|
🤖: Benchmark completed Details
|
That is pretty amazing (3000x-15000x performance improvement 👍 ) I am also testing sql_planner time using |
with_new_childrenPlanProperties, add fast-path for with_new_children
alamb
left a comment
There was a problem hiding this comment.
Thank you @askalt
I went through this carefully -- I think the basic idea is great and we should proceed.
The only thing I think we should try and do is minimize the impact of the API changes when people upgrade. I have some thoughts on that below
For next steps, I suggest break this PR down into smaller parts (it is quite challenging to review all the changes now). Some ideas on relevant chunks:
- Change ExecutionPlan::properties to return an
&Arc<Properties>and the necessary plumbing changes - Changes to FilterExec to avoid cloning project
- Changes to AggregateExec to avoid cloning the vec / exprs
- Changes to the various join operations to avoid cloning the projections
thoughts on reducing upgrade impact
Since this PR does change some core APIs, think we need to be careful.
- I think we can avoid the need to change
Schema::project, see askalt#2 for my proposal - We should figure out some way to make FilterExec::with_projection easier to use -- the new signature is pretty awkward.
Maybe we can use ProjectionExprs or something like
struct SharedProjection {
Arc<[usize]>
}
impl From<Vec<usize>> for SharedProjection {
...
}And then have FilterExec take
pub fn fn with_projection(mut self, projection impl Into<SharedProjection>)
With enough comments I think this could be clear and would also be backwards compatible
| /// This information is available via methods on [`ExecutionPlanProperties`] | ||
| /// trait, which is implemented for all `ExecutionPlan`s. | ||
| fn properties(&self) -> &PlanProperties; | ||
| fn properties(&self) -> &Arc<PlanProperties>; |
There was a problem hiding this comment.
This is technically a breaking API change, so it should be documented in the upgrading.md guide:
https://github.com/apache/datafusion/blob/main/docs/source/library-user-guide/upgrading.md
Per the policy in
https://datafusion.apache.org/contributor-guide/api-health.html
I am happy to help write such an entry
There was a problem hiding this comment.
Entry is added, please review updated upgrading.md.
| name: String, | ||
| plan: FFI_ExecutionPlan, | ||
| properties: PlanProperties, | ||
| properties: Arc<PlanProperties>, |
There was a problem hiding this comment.
since the FFI is supposed to be a stable boundary, I think we shouldn't change this (and instead just copy the properties when needed)
So I suggest reverting this change
There was a problem hiding this comment.
As ExecutionPlan is implemented for ForeignExecutionPlan then we must return &Arc<PlanProperties> from properties(...). So Arc should be owned by someone to return its borrowing. We can change a signature of properties(...) to return owned Arc but cloning seems redundant for me, as the most plans store owned Arc.
Note that FFI_ExecutionPlan is not touched by the patch. Do you sure that it is important to keep ForeignExecutionPlan stable?
/// This struct is used to access an execution plan provided by a foreign
/// library across a FFI boundary.
///
/// The ForeignExecutionPlan is to be used by the caller of the plan, so it has
/// no knowledge or access to the private data. All interaction with the plan
/// must occur through the functions defined in FFI_ExecutionPlan.
#[derive(Debug)]
pub struct ForeignExecutionPlan {
name: String,
plan: FFI_ExecutionPlan,
properties: Arc<PlanProperties>,
children: Vec<Arc<dyn ExecutionPlan>>,
}|
Merging up so I can run planner benchmarks |
|
run benchmark sql_planner |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
I am working on splitting changes into separate PRs, will move changes related to shared projection and aggregates. Changes related to |
ebe2144 to
a8028af
Compare
|
BTW the planner benchmark results in #19792 (comment) show a 1-10% improvement across the board and some queries are going 40% faster (see physical_window_function_partition_by_30_on_values for example) I am going to rerun to see if the results are reproducible |
|
run benchmark sql_planner |
|
🤖 |
|
I removed projection and aggr expr/filters |
|
🤖: Benchmark completed Details
|
a8028af to
fb13e41
Compare
This patch aims to implement a fast-path for the ExecutionPlan::with_new_children function for some plans, moving closer to a physical plan re-use implementation and improving planning performance. If the passed children properties are the same as in self, we do not actually recompute self's properties (which could be costly if projection mapping is required). Instead, we just replace the children and re-use self's properties as-is. To be able to compare two different properties -- ExecutionPlan::properties(...) signature is modified and now returns `&Arc<PlanProperties>`. If `children` properties are the same in `with_new_children` -- we clone our properties arc and then a parent plan will consider our properties as unchanged, doing the same. - Return `&Arc<PlanProperties>` from `ExecutionPlan::properties(...)` instead of a reference. - Implement `with_new_children` fast-path if there is no children properties changes for all major plans. Note: currently, `reset_plan_states` does not allow to re-use plan in general: it is not supported for dynamic filters and recursive queries features, as in this case state reset should update pointers in the children plans. Closes apache#19796
fb13e41 to
509e3ff
Compare
PlanPropertiesredundently #19796This patch aims to implement a fast-path for the ExecutionPlan::with_new_children function for some plans, moving closer to a physical plan re-use implementation and improving planning performance. If the passed children properties are the same as in self, we do not actually recompute self's properties (which could be costly if projection mapping is required). Instead, we just replace the children and re-use self's properties as-is.
To be able to compare two different properties -- ExecutionPlan::properties(...) signature is modified and now returns
&Arc<PlanProperties>. Ifchildrenproperties are the same inwith_new_children-- we clone our properties arc and then a parent plan will consider our properties as unchanged, doing the same.&Arc<PlanProperties>fromExecutionPlan::properties(...)instead of a reference.with_new_childrenfast-path if there is no children properties changes for allmajor plans.
Note: currently,
reset_plan_statesdoes not allow to re-use plan in general: it is notsupported for dynamic filters and recursive queries features, as in this case state reset
should update pointers in the children plans.