*: improve plan cache param eval and insert const #10746

lysu · 2019-06-10T05:03:02Z

What problem does this PR solve?

prepare-plan-cache will generate builtinGetParamStringSig and interpreting running and also do cast from T to string then cast back to a string
almost time insert values .....' s values just const, but we interpreted for each value

What is changed and how it works?

no expression eval, directly get param from session
identity const value insert at parse phase and use the fast way to take the insert values

Check List

Tests

Unit test
Integration test
Bench

Code changes

N/A
Side effects
N/A

Related changes

Need to cherry-pick to the release branch

This change is

lysu · 2019-06-10T05:04:01Z

/bench

codecov · 2019-06-10T05:15:17Z

Codecov Report

Merging #10746 into master will decrease coverage by 0.0071%.
The diff coverage is 88.8235%.

@@               Coverage Diff                @@
##             master     #10746        +/-   ##
================================================
- Coverage   81.4186%   81.4114%   -0.0072%     
================================================
  Files           438        434         -4     
  Lines         94993      93536      -1457     
================================================
- Hits          77342      76149      -1193     
+ Misses        12174      11917       -257     
+ Partials       5477       5470         -7

lysu · 2019-06-10T05:15:59Z

/bench

lysu · 2019-06-10T05:34:36Z

/bench

zhouqiang-cl · 2019-06-19T04:14:09Z

@lysu /bench command is offline now due to unstable aws server. It will be online after about 2 weeks after there is enough server in new IDC

lysu · 2019-06-20T06:25:12Z

/build

ngaut · 2019-07-11T14:27:49Z

Would you like to take a look? @dbjoa

dbjoa · 2019-07-12T03:15:49Z

@lysu
If the cost of accessing the parameter value with Eval() is not significant relatively and know the type of a given parameter in advance, we can remove the unnecessary string-typed casting by adding several typed parameter getting functions (e.g., builtinGetParamNumericSig(), builtinGetParamDatetimeSig(), etc.).

Could you share a benchmark result of the PR?

lysu · 2019-07-12T03:26:18Z

@lysu
If the cost of accessing the parameter value with Eval() is not significant relatively and know the type of a given parameter in advance, we can remove the unnecessary string-typed casting by adding several typed parameter getting functions (e.g., builtinGetParamNumericSig(), builtinGetParamDatetimeSig(), etc.).

Could you share a benchmark result of the PR?

@dbjoa I have a benchmark at #10884 (comment) that combined two PR.

In PrepareStmt parameter should always be a const value datum(which has type), we are better not pay cost to create function
object and calling eval(which will do many type switch), it will be more obvious if we are insert a wide table with many param holder.

The idea is we know param is simple const and we can directly use it, do less eval/cast. 😄

and I will continue to improve this pr..

dbjoa · 2019-07-12T06:40:22Z

@lysu
How about do we specialize Constant.Eval() by changing the small codes like below, which may be able to achieve the similar performance gain like using DeferredParam?

// Eval implements Expression interface.
func (c *Constant) Eval(_ chunk.Row) (dt types.Datum, err error) {
	if c.DeferredExpr != nil {
		if sf, sfOK := c.DeferredExpr.(*ScalarFunction); sfOK {
			if sf.FuncName.O == ast.GetParam {
				idx := sf.GetArgs()[0].(*Constant).Value.GetInt64()
				dt = sf.GetCtx().GetSessionVars().PreparedParams[idx]
			} else {
				dt, err = sf.Eval(chunk.Row{})
				if err != nil {
					return c.Value, err
				}
			}

lysu · 2019-07-12T06:51:01Z

@dbjoa yes, it's better but we also new complex function expression in

tidb/expression/util.go

Line 562 in c8d1ff7

value.DeferredExpr = f

, maybe we can cut off it

zz-jason · 2019-08-05T09:24:16Z

@lysu can we review this PR now?

lysu · 2019-08-07T07:03:37Z

/run-all-tests

crazycs520 · 2019-08-07T08:33:20Z

executor/insert_common.go

-		if err != nil {
-			return err
+		var row []types.Datum
+		if e.notAllConstant {


Try to avoid branch prediction failure.

// put below out of loop. evalRowFunc := e.fastEvalRow if e.notAllConstant { evalRowFunc = e.evalRow }

planner/core/point_get_plan.go

planner/core/planbuilder.go

planner/core/logical_plan_builder.go

planner/core/common_plans.go

expression/expression.go

expression/constant_fold.go

zz-jason · 2019-08-08T07:48:52Z

expression/constant.go

+		if err != nil {
+			return types.Duration{}, true, err
+		}
+		dur, err := types.ParseDuration(ctx.GetSessionVars().StmtCtx, val, types.MaxFsp)


It's better to wrap a cast string as duration function on this deferred param. A method is to transform the deferred param to a expression.Constant, construct a unfolded scalar Cast function upon that Constant.

good catch, can we do this on next PR to avoid useless str<->duration?

we also need change

tidb/server/conn_stmt.go

Line 538 in cb23b52

return pos, fmt.Sprintf("%s%d %02d:%02d:%02d", sign, days, hours, minutes, seconds)

to let dt in here be a duration datum, also some other types

expression/constant.go

executor/update.go

jackysp · 2019-08-08T08:16:09Z

expression/constant.go

@@ -284,7 +412,6 @@ func (c *Constant) EvalJSON(ctx sessionctx.Context, _ chunk.Row) (json.BinaryJSO
 		if err != nil {
 			return json.BinaryJSON{}, true, err
 		}
-		fmt.Println("const eval json", val.GetMysqlJSON().String())


Could you use another PR to remove this line? We need to cherry pick this change to the release branches...

executor/update.go

jackysp

LGTM

jackysp · 2019-08-09T03:14:50Z

PTAL @crazycs520

crazycs520

Rest LGTM

expression/constant.go

zz-jason · 2019-08-09T05:42:31Z

expression/constant.go

@@ -284,7 +412,6 @@ func (c *Constant) EvalJSON(ctx sessionctx.Context, _ chunk.Row) (json.BinaryJSO
 		if err != nil {
 			return json.BinaryJSON{}, true, err
 		}
-		fmt.Println("const eval json", val.GetMysqlJSON().String())


Could you use another PR to remove this line? We need to cherry pick this change to the release branches...

lysu · 2019-08-09T13:40:47Z

/run-all-tests

lysu · 2019-08-10T07:32:14Z

/run-all-tests

lysu · 2019-08-10T07:44:24Z

/run-unit-test

shenli · 2019-08-11T08:08:03Z

Could we cherry-pick this PR to the release-3.0?

jackysp · 2019-08-11T10:48:10Z

/run-unit-test

lysu · 2019-08-13T04:42:19Z

/run-all-tests

lysu · 2019-08-15T02:46:49Z

/run-all-tests

lysu · 2019-08-15T03:13:07Z

@zz-jason ptal if free ~ thx

expression/constant.go

lysu · 2019-08-20T06:57:10Z

/run-all-tests

lysu · 2019-08-21T06:10:06Z

last day, testcase meet a problem: wrong parameter type introduce a cast function and that cast let query can not using right index(cast couldn't push down to tikv now), and that problem has be fixed, but maybe we better keep this in master to protect the risk to introduce other cast function and using wrong index. we can cherry-pick this after more test or after cast can be pushed down to tikv(in roadmap)

PTAL @zz-jason if free, thx

zz-jason

LGTM

zz-jason · 2019-08-26T09:40:22Z

@lysu please merge the master branch and resolve conflicts.

sre-bot · 2019-08-27T03:08:04Z

/run-all-tests

sre-bot · 2019-08-27T03:13:25Z

cherry pick to release-3.0 failed

lysu added component/server status/WIP labels Jun 10, 2019

lysu mentioned this pull request Jun 26, 2019

*: directly save prepare execute args as datums in binary protocol #10884

Merged

lysu requested a review from coocood July 12, 2019 06:51

lysu removed the status/WIP label Aug 7, 2019

lysu marked this pull request as ready for review August 7, 2019 04:26

lysu requested review from jackysp and crazycs520 August 7, 2019 04:27

lysu added the type/enhancement The issue or PR belongs to an enhancement. label Aug 7, 2019

lysu requested a review from zz-jason August 7, 2019 07:10

crazycs520 reviewed Aug 7, 2019

View reviewed changes

zz-jason reviewed Aug 8, 2019

View reviewed changes

jackysp reviewed Aug 8, 2019

View reviewed changes

executor/update.go Show resolved Hide resolved

jackysp reviewed Aug 8, 2019

View reviewed changes

crazycs520 reviewed Aug 9, 2019

View reviewed changes

executor/update.go Outdated Show resolved Hide resolved

jackysp reviewed Aug 9, 2019

View reviewed changes

crazycs520 reviewed Aug 9, 2019

View reviewed changes

zz-jason reviewed Aug 9, 2019

View reviewed changes

jackysp added the needs-cherry-pick-3.0 label Aug 11, 2019

lysu added the status/all tests passed label Aug 15, 2019

crazycs520 reviewed Aug 19, 2019

View reviewed changes

expression/constant.go Outdated Show resolved Hide resolved

crazycs520 reviewed Aug 19, 2019

View reviewed changes

expression/constant.go Show resolved Hide resolved

lysu added the status/DNM label Aug 19, 2019

lysu removed status/DNM labels Aug 20, 2019

lysu requested a review from zz-jason August 26, 2019 05:15

zz-jason reviewed Aug 26, 2019

View reviewed changes

lysu added 2 commits August 27, 2019 11:05

expression,planner: improve prepare plan cache eval param

70499e0

fix unfold cast due type not match

dba61ae

jackysp approved these changes Aug 27, 2019

View reviewed changes

lysu added the status/can-merge Indicates a PR has been approved by a committer. label Aug 27, 2019

jackysp merged commit 4cd5d15 into pingcap:master Aug 27, 2019

*: improve plan cache param eval and insert const #10746

*: improve plan cache param eval and insert const #10746

Conversation

lysu commented Jun 10, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

lysu commented Jun 10, 2019

codecov bot commented Jun 10, 2019 • edited Loading

Codecov Report

lysu commented Jun 10, 2019

lysu commented Jun 10, 2019

zhouqiang-cl commented Jun 19, 2019

lysu commented Jun 20, 2019

ngaut commented Jul 11, 2019

dbjoa commented Jul 12, 2019

lysu commented Jul 12, 2019

dbjoa commented Jul 12, 2019

lysu commented Jul 12, 2019

zz-jason commented Aug 5, 2019

lysu commented Aug 7, 2019

crazycs520 Aug 7, 2019

Choose a reason for hiding this comment

zz-jason Aug 8, 2019

Choose a reason for hiding this comment

lysu Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

jackysp Aug 8, 2019

Choose a reason for hiding this comment

zz-jason Aug 9, 2019

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

jackysp commented Aug 9, 2019

crazycs520 left a comment

Choose a reason for hiding this comment

zz-jason Aug 9, 2019

Choose a reason for hiding this comment

lysu commented Aug 9, 2019

lysu commented Aug 10, 2019

lysu commented Aug 10, 2019

shenli commented Aug 11, 2019

jackysp commented Aug 11, 2019

lysu commented Aug 13, 2019

lysu commented Aug 15, 2019

lysu commented Aug 15, 2019

lysu commented Aug 20, 2019

lysu commented Aug 21, 2019 • edited Loading

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 26, 2019

sre-bot commented Aug 27, 2019

sre-bot commented Aug 27, 2019

lysu commented Jun 10, 2019 •

edited

Loading

codecov bot commented Jun 10, 2019 •

edited

Loading

lysu Aug 9, 2019 •

edited

Loading

lysu commented Aug 21, 2019 •

edited

Loading