Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

executor: improve wide table insert & update performance #7935

Merged
merged 12 commits into from Oct 23, 2018

Conversation

@lysu
Copy link
Member

commented Oct 17, 2018

What problem does this PR solve?

from one of our customers, wide table's INSERT in 2.1 is slower than 2.0

the question is evalRow call chunk.MutRowFromDatums values-len x column-num times, and MutRowFromDatums will make heavy alloc operation

this is cpu pprof result in 58 column insert under dead loop

x

What is changed and how it works?

eval buffer can be reuse and modify partly(if x = 3, y = x + 1 situation)

Check List

Tests

  • Unit test
  • Manual test compare to old code

single thread dead loop to execute a sql insert into 58 columns table and monitor insertValues#insertRows time.

before this PR:

====> 13.343243ms
====> 13.249525ms
====> 12.252023ms
====> 21.257291ms
====> 19.790786ms
====> 12.445035ms
====> 23.729567ms
====> 11.301022ms
====> 12.192163ms
====> 12.253682ms
====> 11.215405ms
====> 11.362502ms

after this PR:

----> 579.105µs
----> 589.978µs
----> 647.785µs
----> 1.184076ms
----> 678.121µs
----> 1.005605ms
----> 565.764µs
----> 617.942µs
----> 628.033µs
----> 569.044µs
----> 575.293µs
----> 605.097µs
----> 569.361µs
----> 935.86µs

Code changes

  • impl change

Side effects

  • no

Related changes

  • Need to cherry-pick to 2.1
  • Need to be included in the release note

Other Question

MutRowFromDatums is devil, we should avoid to use it and delete other usage too


This change is Reviewable

@lysu

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

/run-all-tests

Copy link
Member

left a comment

LGTM

@lysu lysu added the status/DNM label Oct 17, 2018
@ngaut

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Please add a performance test to integrate tests.

@lysu lysu added the require-LGT3 label Oct 18, 2018
@lysu lysu changed the title executor: improve wide table insert performance executor: improve wide table insert/update performance Oct 18, 2018
@lysu lysu changed the title executor: improve wide table insert/update performance executor: improve wide table insert & update performance Oct 18, 2018
@lysu

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

There are some bugs in first version modification.

it works well if expr is const or some expression that doesn't do unsafe operation on chunk.

but when expr is sth like column will meet data corruption, eval will unsafe reuse evalBuffer memory and update wrong data.

so Now, we add Flag() method to expression(use flag because it maybe can provide more information like strickness in furtue) and builtinFunc to indicate whelther we can safe reuse chunk....

the prefect way is modify all builtinFunc impl flag, but it's too much work to do, so now just modify baseBuilitnFuc as

a expression is not reuse chunk memory if all it's children doesn't reuse chunk memory

so, after this PR, sql like insert t values ('1', 2, 2, now()) or update t set a = 1, b =2 where id =1 will be better, but sql like update t1, t2 set t1.v = t2.v where t1.id = t2.id will do like old code.

@zz-jason @tiancaiamao PTAL

@@ -216,6 +240,13 @@ func (e *InsertValues) evalRow(cols []*table.Column, list []expression.Expressio

offset := cols[i].Offset
row[offset], hasValue[offset] = val1, true
if expr.Flag()&expression.FlagChunkReused > 0 {

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Oct 19, 2018

Contributor

The FlagChunkReused makes me confused, if the expression have a reuse flag, than we create a new chunk ?

This comment has been minimized.

Copy link
@lysu

lysu Oct 19, 2018

Author Member

yes, expression reuse chunk, so we can not reuse old chunk for following eval~

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Oct 19, 2018

Contributor

How about expression.HoldChunkMemory, it the returned result of an expression need to hold the memory of the chunk, we need to allocate a new chunk.

expression/builtin.go Outdated Show resolved Hide resolved
@lysu lysu force-pushed the lysu:fix-wide-table-insert-perf branch from cb83989 to c456dce Oct 19, 2018
if e.evalBuffer != nil {
return
}
tpsLen := len(e.Table.Cols())

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 20, 2018

Member

how about s/tpsLen/numCols/

if e.hasExtraHandle {
e.evalBufferTypes[len(e.evalBufferTypes)-1] = types.NewFieldType(mysql.TypeLonglong)
}
mutChunk := chunk.MutRowFromTypes(e.evalBufferTypes)

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 20, 2018

Member

how about s/mutChunk/mutRow/


const (
// FlagHoldChunkMemory indicates expression return result maybe returned by unsafe operation over chunk.
FlagHoldChunkMemory Flag = 1 << iota

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 20, 2018

Member

By default, this is true. I think we can remove this flag and the Flag() API from the expression interface.

This comment has been minimized.

Copy link
@lysu

lysu Oct 22, 2018

Author Member

after macro bench, always copy result in insert_rows is

1.059124ms
1.113391ms
1.037529ms
1.018771ms
998.436µs
1.010835ms
929.046µs
1.502075ms
972.367µs
984.169µs
990.929µs
967.04µs
1.797491ms
1.506823ms
957.703µs
979.513µs
1.001699ms
1.062672ms

it's litte differ to not-copy-flag solution, so we choose always copy now

PTAL @tiancaiamao @zz-jason

@lysu lysu force-pushed the lysu:fix-wide-table-insert-perf branch from f98115b to afd5218 Oct 22, 2018
@@ -47,7 +47,7 @@ func (col *CorrelatedColumn) Eval(row chunk.Row) (types.Datum, error) {
}

// EvalInt returns int representation of CorrelatedColumn.
func (col *CorrelatedColumn) EvalInt(ctx sessionctx.Context, row chunk.Row) (int64, bool, error) {
func (col *CorrelatedColumn) EvalInt(ctx sessionctx.Context, _ chunk.Row) (int64, bool, error) {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 22, 2018

Member

the change of this file is irrelevant, can we revert it back?

@@ -141,6 +142,8 @@ func (e *UpdateExec) fetchChunkRows(ctx context.Context) error {
fields := e.children[0].retTypes()
globalRowIdx := 0
chk := e.children[0].newFirstChunk()
mutChunk := chunk.MutRowFromTypes(fields)

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 22, 2018

Member

s/mutChunk/mutRow/

@@ -42,6 +42,7 @@ type UpdateExec struct {
// columns2Handle stores relationship between column ordinal to its table handle.
// the columns ordinals is present in ordinal range format, @see executor.cols2Handle
columns2Handle cols2HandleSlice
evalBuffer *chunk.MutRow

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 22, 2018

Member

evalBuffer can be declared as chunk.MutRow

@@ -49,11 +49,12 @@ func newBaseBuiltinFunc(ctx sessionctx.Context, args []Expression) baseBuiltinFu
if ctx == nil {
panic("ctx should not be nil")
}
return baseBuiltinFunc{
fn := baseBuiltinFunc{

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 22, 2018

Member

the change of this file is irrelevant, can we revert it back?

@@ -36,6 +36,14 @@ const (
scalarFunctionFlag byte = 3
)

// Flag represents expression's property.
type Flag byte

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 22, 2018

Member

can be removed?

This comment has been minimized.

Copy link
@zhexuany

zhexuany Oct 23, 2018

Member

It seems to be used in next few lines. FlagHoldChunkMemory Flag = 1 << iota

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 23, 2018

Member

Yeah, I mean this line and the flowing few lines can all be removed.

@@ -117,9 +119,30 @@ func (e *InsertValues) getColumns(tableCols []*table.Column) ([]*table.Column, e
return nil, errors.Trace(err)
}

e.initEvalBuffer()

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 22, 2018

Member

This function call can be put to InsertExec.Open()?

This comment has been minimized.

Copy link
@lysu

lysu Oct 23, 2018

Author Member

maybe not convience to do that, because we need e.hasExtraHandle which is assigned in getColumns now.

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 23, 2018

Member

getColumns() is called in the Next() function, Maybe we can move it to the Open() function:

InsertExec.Open:
	getColumns()
	initEvalBuffer()

InsertExec.Next:
	e.insertRowsFromSelect(), or:
	e.insertRows()

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 23, 2018

Member

another thing is, according to the callers of the function getColumns(), maybe we can also improve the performance of the replace statement:

  3 executor/insert.go|132 col 17| cols, err := e.getColumns(e.Table.Cols())
  4 executor/replace.go|181 col 17| cols, err := e.getColumns(e.Table.Cols())
  5 executor/builder.go|576 col 28| columns, err := insertVal.getColumns(tableCols)

This comment has been minimized.

Copy link
@lysu

lysu Oct 23, 2018

Author Member

finally, I move getColumns to buildExecutor just like loadData does...and move initEvalBuffer to Open, and replace pointer to direct ref.

PTAL @zz-jason , although CI doesn't work

@lysu lysu force-pushed the lysu:fix-wide-table-insert-perf branch from b5e19c4 to a2340dc Oct 23, 2018
@lysu

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

/run-all-tests

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

LGTM @zz-jason

@@ -50,9 +50,13 @@ type InsertValues struct {
GenColumns []*ast.ColumnName
GenExprs []expression.Expression

InsertColumns []*table.Column

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 23, 2018

Member

no need to export it? s/InsertColumns/insertColumns/

@@ -114,10 +120,25 @@ func (e *InsertValues) getColumns(tableCols []*table.Column) ([]*table.Column, e
// Check column whether is specified only once.
err = table.CheckOnce(cols)
if err != nil {
return nil, errors.Trace(err)
return errors.Trace(err)

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 23, 2018

Member

no need to Trace any more?

@lysu lysu force-pushed the lysu:fix-wide-table-insert-perf branch from b13df8d to 99deb30 Oct 23, 2018
@lysu

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

/run-all-tests PTAL @zz-jason

Copy link
Member

left a comment

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Oct 23, 2018
@zz-jason

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

/run-all-tests

zz-jason added 2 commits Oct 23, 2018
@iamxy

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

/run-all-tests

@ngaut ngaut merged commit d8b52b8 into pingcap:master Oct 23, 2018
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
iamzhoug37 added a commit to iamzhoug37/tidb that referenced this pull request Oct 25, 2018
bugfix fixed pingcap#7518

expression: MySQL compatible current_user function (pingcap#7801)

plan: propagate constant over outer join (pingcap#7794)

- extract `outerCol = const` from join conditions and filter conditions,
  substitute `outerCol` in join conditions with `const`;
- extract `outerCol = innerCol` from join conditions, derive new join
  conditions based on this column equal condition and `outerCol` related
  expressions in join conditions and filter conditions;

util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901)

stats: handle ddl event for partition table (pingcap#7903)

plan: implement Operand and Pattern of cascades planner. (pingcap#7910)

planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808)

plan: move projEliminate behind aggEliminate (pingcap#7909)

admin: fix admin check table bug of byte compare (pingcap#7887)

* admin: remove reflect deepEqual

stats: fix panic caused by empty histogram (pingcap#7912)

plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906)

* fix drop view if exist error (pingcap#7833)

executor: refine `explain analyze` (pingcap#7888)

executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863)

expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915)

stats: fix histogram boundaries overflow error (pingcap#7883)

ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771)

* ddl:support the definition of null change to not null using alter table

ddl: add check when create table with foreign key. (pingcap#7885)

* ddl: add check when create table with foreign key

planner: eliminate if null on non null column (pingcap#7924)

executor: fix a bug in point get (pingcap#7934)

planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930)

executor: fix panic when limit is too large (pingcap#7936)

ddl: add TiDB version to metrics (pingcap#7902)

stats: limit the length of sample values (pingcap#7931)

vendor: update tipb (pingcap#7893)

planner: support the Group and GroupExpr for the cascades planner (pingcap#7917)

store/tikv: log more information when other err occurs (pingcap#7948)

types: fix date time parse (pingcap#7933)

ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875)

stats: update delta info for partition table (pingcap#7947)

explaintest: add explain test for partition pruning (pingcap#7505)

util: move disjoint set to util package (pingcap#7950)

util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916)

executor: add the slow log for commit (pingcap#7951)

expression: add builtin json_keys (pingcap#7776)

privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955)

ddl: fix invailid ddl job panic (pingcap#7940)

*: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952)

Make the ast package get rid of the dependency of types.Datum

server: allow cors http request (pingcap#7939)

*: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970)

pr suggestion update

executor/aggfuncs: split unit tests to corresponding file (pingcap#7993)

store/tikv: fix typo (pingcap#7990)

executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999)

executor: let information_schema be the first database in ShowDatabases (pingcap#7938)

stats: use local feedback for partition table (pingcap#7963)

executor: add unit test for aggfuncs (pingcap#7966)

server: add log for binary execute statement (pingcap#7987)

admin: refine admin check decoder (pingcap#7862)

executor: improve wide table insert & update performance (pingcap#7935)

ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919)

fix reassigned partition id in truncate table does not take effect

add changelog for 2.1.0 rc4 (pingcap#8020)

*: make parser package dependency as small as possible (pingcap#7989)

parser: support `:=` in the `set` syntax (pingcap#8018)

According to MySQL document, `set` use the = assignment operator,
but the := assignment operator is also permitted

stats: garbage collect stats for partition table (pingcap#7962)

docs: add the proposal for the column pool (pingcap#7988)

expression: refine built-in func truncate to support uint arg (pingcap#8000)

stats: support show stats for partition table (pingcap#8023)

stats: update error rate for partition table (pingcap#8022)

stats: fix estimation for out of range point queries (pingcap#8015)

*: move parser to a separate repository (pingcap#8036)

executor: fix wrong result when index join on union scan. (pingcap#8031)

Do not modify Plan of dataReaderBuilder directly, because it would
impact next batch of outer rows, as well as other concurrent inner
workers. Instead, build a local child builder to store the child plan.

planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017)

*: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044)

* ddl, executor: fix the issue of executing DDL after executing SQL failure in txn

add unit test

remove debug info

add like evaluator case sensitive test

ddl, domain: make schema correct after canceling jobs (pingcap#7997)

unit test fix

code format

proposal: maintaining histograms in plan. (pingcap#7605)

support _tidb_rowid for table scan range (pingcap#8047)

var rename fix
lysu added a commit to lysu/tidb that referenced this pull request Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.