feat(optimizer): improve inline now proc time #13609

chenzl25 · 2023-11-23T03:31:18Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Related issue Sysbench performance bottleneck analysis -- point select #13459 feat: support ExprVisitable for plan nodes #13510
Avoid rewrite of InlineNowProcTime for plan nodes if there are no now() and proctime().
Performance comparison feat(optimizer): support expr visitable for plan node #13587 (comment)

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

codecov · 2023-11-23T05:27:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (38a78d8) 68.06% compared to head (195bb5c) 68.07%.
Report is 11 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13609   +/-   ##
=======================================
  Coverage   68.06%   68.07%           
=======================================
  Files        1516     1516           
  Lines      261570   261595   +25     
=======================================
+ Hits       178049   178082   +33     
+ Misses      83521    83513    -8

Flag	Coverage Δ
rust	`68.07% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xxchan · 2023-11-23T06:59:55Z

src/frontend/src/optimizer/logical_optimization.rs

@@ -487,10 +491,16 @@ impl LogicalOptimizer {
    }

    pub fn inline_now_proc_time(plan: PlanRef, ctx: &OptimizerContextRef) -> PlanRef {
-        // TODO: if there's no `NOW()` or `PROCTIME()`, we don't need to acquire snapshot.
+        let mut v = InlineNowProcTime::new(Epoch(INVALID_EPOCH));
+        plan.visit_exprs_recursive(&mut v);


Maybe better to add doc comment on visit_exprs_recursive and rewrite_exprs_recursive, mentioning such usage is for performance.

BugenZhao · 2023-11-23T07:08:36Z

Actually I'm wondering why this could improve the performance. It appears that a no-op call of rewrite_exprs_recursive will still lead to performance overhead of cloning and allocation, which is far from ideal. In this PR, we manually add a visitor dual for a rewriter to avoid calling rewrite as much as possible. However, the developers still have to make sure they're identical, just like what we talked in #13587 (comment).

I'm considering whether it's possible to reduce the overhead of rewrite...

We already have has_rewritable_expr to reduce the calls.

risingwave/src/frontend/src/optimizer/plan_node/expr_rewritable.rs

Lines 26 to 28 in d37b87b

fn has_rewritable_expr(&self) -> bool {

false

}

Is it possible to make it specific to different rewriters? This will somehow be similar to https://doc.rust-lang.org/stable/std/error/struct.Request.html#method.would_be_satisfied_by_ref_of.

When expressions not actually be rewritten, rewrite_exprs should be able to directly return itself without cloning. This can be achieved with Cow.

risingwave/src/frontend/src/optimizer/plan_node/logical_project.rs

Lines 172 to 180 in d37b87b

    
           fn rewrite_exprs(&self, r: &mut dyn ExprRewriter) -> PlanRef { 
        
               let mut core = self.core.clone(); 
        
               core.rewrite_exprs(r); 
        
               Self { 
        
                   base: self.base.clone_with_new_plan_id(), 
        
                   core, 
        
               } 
        
               .into() 
        
           }

Similarly when inputs not changed, clone_with_inputs should be able to directly return itself without cloning. This can be achieved with Rc::ptr_eq.

risingwave/src/frontend/src/optimizer/plan_node/mod.rs

Line 300 in d37b87b

new.clone_with_inputs(&inputs[..])

There is no need to allocate a Vec<ExprImpl> or clone the expression when rewriting them.

risingwave/src/frontend/src/optimizer/plan_node/generic/project.rs

Lines 55 to 63 in d37b87b

    
           impl<PlanRef> Project<PlanRef> { 
        
               pub(crate) fn rewrite_exprs(&mut self, r: &mut dyn ExprRewriter) { 
        
                   self.exprs = self 
        
                       .exprs 
        
                       .iter() 
        
                       .map(|e| r.rewrite_expr(e.clone())) 
        
                       .collect(); 
        
               } 
        
           }

Take this for an example, a more efficient version should be...

        for expr in self.exprs.iter_mut() {
            *expr = r.rewrite_expr(std::mem::replace(expr, ExprImpl::Dummy));
        }

where no allocation will be done if there's no actual rewriting.

st1page · 2023-11-23T07:09:54Z

src/frontend/planner_test/tests/testdata/output/explain.yaml

@@ -55,7 +50,7 @@
      "stages": {
        "0": {
          "root": {
-            "plan_node_id": 10016,
+            "plan_node_id": 10013,


It somehow becomes a indicator about how many plan node has been generated during the optimization 😆

BugenZhao · 2023-11-23T07:10:09Z

src/frontend/src/expr/now.rs

@@ -81,3 +91,21 @@ impl ExprRewriter for InlineNowProcTime {
        FunctionCall::new_unchecked(func_type, inputs, ret).into()
    }
 }
+
+impl ExprVisitor for InlineNowProcTime {


I would suggest defining a new visitor type since its behavior is strictly orthogonal to the rewriter.

BugenZhao

This PR LGTM to achieve goal #13459.

I agree with Bugen, so wait for his approval instead

st1page · 2023-11-23T07:35:59Z

When expressions not actually be rewritten, rewrite_exprs should be able to directly return itself without cloning. This can be achieved with Cow.

risingwave/src/frontend/src/optimizer/plan_node/logical_project.rs

Lines 172 to 180 in d37b87b

fn rewrite_exprs(&self, r: &mut dyn ExprRewriter) -> PlanRef {

let mut core = self.core.clone();

core.rewrite_exprs(r);

Self {

base: self.base.clone_with_new_plan_id(),

core,

}

.into()

}

I guess we can achieve it with passing the Arc

use std::rc::Rc;
pub trait ExprRewritable {
    fn rewrite_exprs(self: Rc<Self>) -> Rc<dyn ExprRewritable>;
}

struct Proj {
    a: i32
}

impl ExprRewritable for Proj {
    fn rewrite_exprs(self: Rc<Self>) -> Rc<dyn ExprRewritable> {
        if self.a == 1 {
            return Rc::new(Proj{ a: 2});
        } else {
            return self
        }
    }

}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=af96f135c7068c575bd1a0d9493b7ee3

chenzl25 · 2023-11-23T08:50:35Z

Actually I'm wondering why this could improve the performance. It appears that a no-op call of rewrite_exprs_recursive will still lead to performance overhead of cloning and allocation, which is far from ideal.

Good idea. Maybe we can refactor it in the future if necessary.

In this PR, we manually add a visitor dual for a rewriter to avoid calling rewrite as much as possible.
However, the developers still have to make sure they're identical, just like what we talked in #13587 (comment).

I think letting developers make sure they're identical in our cases seems trivial. It is not too painful to work with it at this moment.

chenzl25 added 3 commits November 22, 2023 16:05

support expr visitable for plan node

6ac2934

fmt

3e86ae4

improve inline now proc time

a9cf123

github-actions bot added the type/feature label Nov 23, 2023

Base automatically changed from dylan/support_expr_visitable_for_plan_node to main November 23, 2023 03:40

Merge branch 'main' into dylan/improve_inline_now_proc_time

ed5845c

chenzl25 requested review from BugenZhao, xiangjinwu and xxchan November 23, 2023 03:42

Merge branch 'main' into dylan/improve_inline_now_proc_time

36ae9a5

xxchan previously approved these changes Nov 23, 2023

View reviewed changes

st1page reviewed Nov 23, 2023

View reviewed changes

BugenZhao reviewed Nov 23, 2023

View reviewed changes

chenzl25 requested a review from BugenZhao November 23, 2023 09:05

refine

195bb5c

chenzl25 mentioned this pull request Nov 23, 2023

refactor: better ExprRewritable implementation #13620

Open

BugenZhao approved these changes Nov 24, 2023

View reviewed changes

chenzl25 added this pull request to the merge queue Nov 24, 2023

Merged via the queue into main with commit 937e099 Nov 24, 2023
6 of 7 checks passed

chenzl25 deleted the dylan/improve_inline_now_proc_time branch November 24, 2023 06:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(optimizer): improve inline now proc time #13609

feat(optimizer): improve inline now proc time #13609

chenzl25 commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 23, 2023 •

edited

Loading

xxchan Nov 23, 2023

BugenZhao commented Nov 23, 2023

st1page Nov 23, 2023

BugenZhao Nov 23, 2023

BugenZhao left a comment

st1page commented Nov 23, 2023 •

edited

Loading

chenzl25 commented Nov 23, 2023

feat(optimizer): improve inline now proc time #13609

feat(optimizer): improve inline now proc time #13609

Conversation

chenzl25 commented Nov 23, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

codecov bot commented Nov 23, 2023 • edited Loading

Codecov Report

xxchan Nov 23, 2023

Choose a reason for hiding this comment

BugenZhao commented Nov 23, 2023

st1page Nov 23, 2023

Choose a reason for hiding this comment

BugenZhao Nov 23, 2023

Choose a reason for hiding this comment

BugenZhao left a comment

Choose a reason for hiding this comment

st1page commented Nov 23, 2023 • edited Loading

chenzl25 commented Nov 23, 2023

chenzl25 commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 23, 2023 •

edited

Loading

st1page commented Nov 23, 2023 •

edited

Loading