executor: reduce chunk capacity #7141

lysu · 2018-07-24T09:06:43Z

What have you changed? (mandatory)

ref #7077 2 of 3 reduce chunk size.

At first, separate concept InitChunkSize from maxChunkSize, use initChunkSize to allocate less memory, maxChunkSize to keep maintain "max return data" mean.

After that, we should choose the value of initChunkSize. for now

only callee operator know well about how much min buffer needed for this operator(caller only see the interface, but callee know who it is, e.g. it's an update operator and no need buffer any more).
but caller know well about how returned buffer's usage, e.g. reusable? renew every time? in loop or use once?

so we need return initChunkSize for different operator's via executor#newChunk, and take care about executor#newChunk or chunk.NewChunk*() callee's usage.

In summary, initChunkSize for each operator

to operator that no need chunk, use void-chunk(cap=-1)
to operator that need fixed chunk, use fixed chunk
for reusable chunk, extend them by mechanism of go slice
for unreusable chunk, need renew chunk in loop, will determine next capacity size by previous turn.

What is the type of the changes? (mandatory)

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

unit tests
integration tests

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

WIP

Refer to a related PR or issue link (optional)

#7077

Benchmark result if necessary (optional)

WIP

This change is

lysu · 2018-08-02T14:17:57Z

PTAL if free @coocood @zz-jason @XuHuaiyu ~ thx

winoros

Is there any performance result?

winoros · 2018-08-02T18:33:09Z

config/config.go

@@ -316,6 +317,7 @@ var defaultConf = Config{
 	Binlog: Binlog{
 		WriteTimeout: "15s",
 	},
+	BigChunkThreshold: 512,


Previous, it's 1024?

no~ this is a new added configure. in previous, initChunkSize is 1024, but now initChunkSize is 32 if stats info rows < BigChunkThreshold, else keep 1024

winoros · 2018-08-02T18:34:19Z

executor/executor.go

@@ -134,6 +147,18 @@ func newBaseExecutor(ctx sessionctx.Context, schema *expression.Schema, id strin
 	return e
 }

+type baseNoResultExecutor struct {


What is this?

this is a sub-executor to override newChunk() method to make updateExec/insertExec new a void chunk~

Why we need this?

winoros · 2018-08-02T18:36:19Z

util/mock/context.go

@@ -224,6 +224,7 @@ func NewContext() *Context {
 		cancel:      cancel,
 	}
 	sctx.sessionVars.MaxChunkSize = 2
+	sctx.sessionVars.MaxChunkSize = 1


Why set it 1 after setting it 2?

orz....it's mistake...

zz-jason · 2018-08-03T04:43:00Z

executor/aggregate.go

-			e.executed = true
-			return errors.Trace(err)
-		}
+	if e.executed || chk.NumRows() >= e.maxChunkSize {


what does chk.NumRows() >= e.maxChunkSize mean?

oh....I shouldn't modify that..- -

zz-jason · 2018-08-03T04:46:47Z

util/chunk/list.go

-	chunks       []*Chunk
-	freelist     []*Chunk
+	fieldTypes    []*types.FieldType
+	initChunkSize int


why need this?

lysu · 2018-08-06T12:00:39Z

PTAL @zz-jason @coocood @winoros thx~

coocood · 2018-08-06T12:57:25Z

util/chunk/list.go

-	return NewChunkWithCapacity(l.fieldTypes, l.maxChunkSize)
+	newChk := NewChunkWithCapacity(l.fieldTypes, l.chunkCap)
+	if l.chunkCap < l.maxChunkSize {
+		l.chunkCap <<= 1


*= 2 is easier to read.

coocood · 2018-08-06T13:07:21Z

util/chunk/chunk.go

@@ -40,6 +40,9 @@ const (
 // NewChunkWithCapacity creates a new chunk with field types and capacity.
 func NewChunkWithCapacity(fields []*types.FieldType, cap int) *Chunk {
 	chk := new(Chunk)
+	if cap < 0 {


Should add comments about negative capacity.

coocood · 2018-08-06T13:08:34Z

ast/ast.go

 	NewChunk() *chunk.Chunk

+	// NewChunk creates a new chunk with given capacity.


Should add comments about negative capacity.

zhexuany · 2018-08-06T23:57:43Z

executor/index_lookup_join.go

-func (ow *outerWorker) buildTask(ctx context.Context) (*lookUpJoinTask, error) {
-	ow.executor.newChunk()
-
+func (ow *outerWorker) buildTask(ctx context.Context, preTask *lookUpJoinTask) (*lookUpJoinTask, error) {


We change the method signature, how about change the comment at the same time?

zhexuany · 2018-08-07T00:07:07Z

executor/builder.go

@@ -1184,6 +1201,7 @@ func (b *executorBuilder) buildExists(v *plan.PhysicalExists) Executor {
 	e := &ExistsExec{
 		baseExecutor: newBaseExecutor(b.ctx, v.Schema(), v.ExplainID(), childExec),
 	}
+	e.chunkCap = 1


If just assigning a const to a variable of a struct, I feel like it is better to put it as part of the creation of such struct.

zhexuany · 2018-08-07T00:07:19Z

executor/builder.go

@@ -1196,6 +1214,7 @@ func (b *executorBuilder) buildMaxOneRow(v *plan.PhysicalMaxOneRow) Executor {
 	e := &MaxOneRowExec{
 		baseExecutor: newBaseExecutor(b.ctx, v.Schema(), v.ExplainID(), childExec),
 	}
+	e.chunkCap = 1


same as above

zhexuany · 2018-08-07T00:08:30Z

I think we need a benchmark for this change.

winoros · 2018-08-07T08:20:36Z

ast/ast.go

 	NewChunk() *chunk.Chunk

+	// NewChunk creates a new chunk with given capacity.


It's NewChunkWithCapacity

winoros · 2018-08-07T08:37:46Z

So chunkCap is something the same with initChunkSize?

lysu · 2018-08-07T10:14:12Z

@winoros yes, it's better to use initChunkSize as the name - -

coocood · 2018-08-07T10:33:26Z

statistics/statistics_test.go

@@ -94,11 +94,15 @@ func (r *recordSet) Next(ctx context.Context, chk *chunk.Chunk) error {
 }

 func (r *recordSet) NewChunk() *chunk.Chunk {
+	return r.NewChunkWithCapacity(32)


should use chunk.InitialCapacity.

jackysp · 2018-08-08T06:08:36Z

executor/builder.go

 		ByItems:      v.ByItems,
 		schema:       v.Schema(),
 	}
 	metrics.ExecutorCounter.WithLabelValues("TopNExec").Inc()
-	return &TopNExec{
+	e := &TopNExec{


Why change this?

ok, that isn't no need after address @zhexuany's comment

coocood · 2018-08-08T06:24:14Z

model/model.go

@@ -141,6 +142,7 @@ type TableInfo struct {

 // GetPartitionInfo returns the partition information.
 func (t *TableInfo) GetPartitionInfo() *PartitionInfo {
+	fmt.Println()


remove this line

jackysp · 2018-08-08T06:25:08Z

model/model.go

@@ -141,6 +142,7 @@ type TableInfo struct {

 // GetPartitionInfo returns the partition information.
 func (t *TableInfo) GetPartitionInfo() *PartitionInfo {
+	fmt.Println()


Why add this line?

jackysp · 2018-08-08T06:30:22Z

server/conn.go

@@ -1084,6 +1085,13 @@ func (cc *clientConn) writeChunksWithFetchSize(ctx context.Context, rs ResultSet
 	return errors.Trace(cc.writeEOF(serverStatus))
 }

+func (cc *clientConn) chunkCapacity(wantSize int) int {
+	if wantSize > 1024 {


How about use DefMaxChunkSize here?

jackysp · 2018-08-08T06:34:25Z

util/chunk/list.go

-	return NewChunkWithCapacity(l.fieldTypes, l.maxChunkSize)
+	newChk := NewChunkWithCapacity(l.fieldTypes, l.initChunkSize)
+	if l.initChunkSize < l.maxChunkSize {
+		l.initChunkSize *= 2


It is interesting that some parts use *=2 and others use <<= 1.

coocood · 2018-08-08T06:33:48Z

executor/sort.go

-		childKeyChk = chunk.NewChunkWithCapacity(e.keyTypes, e.maxChunkSize)
+		childKeyChk = chunk.NewChunkWithCapacity(e.keyTypes, e.initChunkSize)
+		if e.initChunkSize < e.maxChunkSize {
+			e.initChunkSize <<= 1


*= 2 is easier to read.

coocood · 2018-08-08T06:34:13Z

executor/index_lookup_join.go

 		lookupMap:         mvmap.NewMVMap(),
 	}
+	if ow.initChunkSize < ow.ctx.GetSessionVars().MaxChunkSize {
+		ow.initChunkSize <<= 1


*= 2 is easier to read.

jackysp · 2018-08-08T06:46:46Z

sessionctx/variable/sysvar.go

@@ -636,6 +636,7 @@ var defaultSysVars = []*SysVar{
 	{ScopeSession, TiDBDMLBatchSize, strconv.Itoa(DefDMLBatchSize)},
 	{ScopeSession, TiDBCurrentTS, strconv.Itoa(DefCurretTS)},
 	{ScopeGlobal | ScopeSession, TiDBMaxChunkSize, strconv.Itoa(DefMaxChunkSize)},
+	{ScopeGlobal | ScopeSession, TiDBInitChunkSize, strconv.Itoa(DefInitChunkSize)},


Is it necessary to add a variable for it?

maybe sometime we need online tunning? I'm not sure~

jackysp · 2018-08-08T06:51:20Z

server/conn.go

@@ -1084,6 +1086,13 @@ func (cc *clientConn) writeChunksWithFetchSize(ctx context.Context, rs ResultSet
 	return errors.Trace(cc.writeEOF(serverStatus))
 }

+func (cc *clientConn) chunkCapacity(wantSize int) int {
+	if wantSize > variable.DefMaxChunkSize {


Ah, I've got a mistake. We should return the current max chunk size here, instead of the default one. And in the next line the return value should not be init chunk size.

zz-jason · 2018-08-08T06:56:48Z

executor/builder.go

@@ -1064,7 +1070,7 @@ func (b *executorBuilder) buildTableDual(v *plan.PhysicalTableDual) Executor {
 		return nil
 	}
 	e := &TableDualExec{
-		baseExecutor: newBaseExecutor(b.ctx, v.Schema(), v.ExplainID()),
+		baseExecutor: newBaseExecutorWithInitChunkSize(b.ctx, v.Schema(), v.ExplainID(), 1),


better to s/1/v.rowCount/

zz-jason · 2018-08-08T07:05:13Z

executor/delete.go

@@ -133,6 +133,7 @@ func (e *DeleteExec) deleteSingleTableByChunk(ctx context.Context) error {
 			}
 			rowCount++
 		}
+		chk = e.children[0].newChunkWithCapacity(chk.NumRows())


If the child operator's initial chunk size is 32, the newly allocated chunk will always be 32 sized?

if the child's initial size = 32, it will allocate 32 in the first loop and after Next() return chk NumRows will be both <=32 or >32, so in the 2nd loop will use numRows as capacity if it works without sth wrong

lysu · 2018-08-09T13:00:14Z

/run-all-tests

lysu

Add some explain in code @zz-jason @coocood

lysu · 2018-08-20T07:40:16Z

ast/ast.go

 	NewChunk() *chunk.Chunk

+	// NewFixedChunk creates a new chunk with given capacity.
+	NewFixedChunk(cap int) *chunk.Chunk


we added a new one parameter ctor, to help renew a chunk.

why not use chunk.Renew()? some stituation using resultSet is out of executor and not easy to get retTypes()

lysu · 2018-08-20T07:41:07Z

cmd/benchdb/main.go

@@ -126,6 +126,7 @@ func (ut *benchDB) mustExec(sql string) {
 			if chk.NumRows() == 0 {
 				break
 			}
+			chk.GrowIfFull()


one reusable chunk, we only need growIfFull previous loop full.

lysu · 2018-08-20T07:41:25Z

ddl/db_test.go

@@ -3167,7 +3167,7 @@ func (s *testDBSuite) TestPartitionDropIndex(c *C) {
 LOOP:
 	for {
 		select {
-		case err := <-done:
+		case err = <-done:


fix make dev vet check

lysu · 2018-08-20T07:42:24Z

ddl/util/util.go

@@ -83,6 +83,7 @@ func LoadDeleteRanges(ctx sessionctx.Context, safePoint uint64) (ranges []DelRan
 				EndKey:    endKey,
 			})
 		}
+		chk.GrowIfFull()


reusable chunk, need grow

lysu · 2018-08-20T07:44:27Z

distsql/select_result.go

@@ -122,7 +122,7 @@ func (r *selectResult) NextRaw(ctx context.Context) ([]byte, error) {
 // Next reads data to the chunk.
 func (r *selectResult) Next(ctx context.Context, chk *chunk.Chunk) error {
 	chk.Reset()
-	for chk.NumRows() < r.ctx.GetSessionVars().MaxChunkSize {
+	for chk.NumRows() < chk.MaxRows() {


make sure Next return max rows(reuse chunk property) passed by outer.

lysu · 2018-08-20T14:34:21Z

executor/merge_join.go

@@ -172,6 +172,7 @@ func (t *mergeJoinInnerTable) hasNullInJoinKey(row chunk.Row) bool {
 func (t *mergeJoinInnerTable) reallocReaderResult() {
 	if !t.curResultInUse {
 		// If "t.curResult" is not in use, we can just reuse it.
+		t.curResult.GrowIfFull()


grow if is reused readerResult, more use time more huge in MJ

lysu · 2018-08-20T14:35:27Z

executor/join.go

@@ -365,6 +366,7 @@ func (e *HashJoinExec) runJoinWorker(workerID uint) {
 		if !ok {
 			break
 		}
+		outerResult.GrowIfFull()


HJ's outer is reusable

lysu · 2018-08-20T14:37:03Z

executor/join.go

@@ -613,6 +615,9 @@ func (e *NestedLoopApplyExec) fetchSelectedOuterRow(ctx context.Context, chk *ch
 	outerIter := chunk.NewIterator4Chunk(e.outerChunk)
 	for {
 		if e.outerChunkCursor >= e.outerChunk.NumRows() {
+			if e.outerChunk.NumRows() > 0 {
+				e.outerChunk.GrowIfFull()
+			}


nestApply pull data as a reuse block, but later block can be bigger

lysu · 2018-08-20T14:37:51Z

executor/join.go

@@ -667,6 +672,7 @@ func (e *NestedLoopApplyExec) fetchAllInners(ctx context.Context) error {
 				e.innerList.AppendRow(row)
 			}
 		}
+		e.innerChunk.GrowIfFull()


nestapply inner data can be reuse(because transfomr to datarow)

lysu · 2018-08-20T14:47:17Z

executor/merge_join.go

@@ -349,6 +351,9 @@ func (e *MergeJoinExec) fetchNextInnerRows() (err error) {
 // may not all belong to the same join key, but are guaranteed to be sorted
 // according to the join key.
 func (e *MergeJoinExec) fetchNextOuterRows(ctx context.Context) (err error) {
+	if e.outerTable.chk.NumRows() > 0 {
+		e.outerTable.chk.GrowIfFull()
+	}


MJ's outer need grow..

lysu · 2018-08-21T03:17:26Z

executor/distsql.go

+		resultCh:      e.resultCh,
+		keepOrder:     e.keepOrder,
+		initChunkSize: e.initChunkSize,
+		maxBatchSize:  e.ctx.GetSessionVars().IndexLookupSize,


batchSize is impl by chunk.MaxRows

lysu · 2018-08-21T03:18:00Z

/run-all-tests

coocood · 2018-08-21T03:41:29Z

util/chunk/chunk.go

+	}
+	c.numVirtualRows = 0
+	for _, col := range c.columns {
+		col.grow(c.maxRows, lastNumRows)


Isn't the lastNumRows is the same as col.length?

it's same i will fix it

lysu · 2018-08-21T05:58:57Z

/run-all-tests

winoros · 2018-08-21T10:23:22Z

In distsql/select_result.go, the SelectResult directly use MaxChunkSize will this affect the chunk size?

lysu · 2018-08-21T12:00:56Z

/run-all-tests

lysu · 2018-09-27T04:12:16Z

PR splited.

lysu added the status/WIP label Jul 24, 2018

lysu changed the title ~~executor: make different executors create more smaller as they can~~ executor: make different executors create more smaller chunk as they can Jul 24, 2018

lysu changed the title ~~executor: make different executors create more smaller chunk as they can~~ executor: separate InitChunkSize from MaxChunkSize and InitChunkSize be 32 Aug 2, 2018

lysu added the sig/execution SIG execution label Aug 2, 2018

lysu changed the title ~~executor: separate InitChunkSize from MaxChunkSize and InitChunkSize be 32~~ executor: separate InitChunkSize from MaxChunkSize Aug 2, 2018

lysu added type/enhancement The issue or PR belongs to an enhancement. and removed status/WIP labels Aug 2, 2018

winoros reviewed Aug 2, 2018

View reviewed changes

winoros added the status/all tests passed label Aug 2, 2018

zz-jason reviewed Aug 3, 2018

View reviewed changes

util/chunk/list.go Outdated

chunks []*Chunk

freelist []*Chunk

fieldTypes []*types.FieldType

initChunkSize int

Copy link

Member

zz-jason Aug 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need this?

lysu added status/WIP and removed status/all tests passed labels Aug 3, 2018

lysu changed the title ~~executor: separate InitChunkSize from MaxChunkSize~~ executor: reduce chunk capacity Aug 6, 2018

coocood reviewed Aug 6, 2018

View reviewed changes

zz-jason self-assigned this Aug 6, 2018

coocood reviewed Aug 6, 2018

View reviewed changes

zhexuany reviewed Aug 6, 2018

View reviewed changes

zhexuany reviewed Aug 7, 2018

View reviewed changes

winoros reviewed Aug 7, 2018

View reviewed changes

ast/ast.go Outdated

NewChunk() *chunk.Chunk

// NewChunk creates a new chunk with given capacity.

Copy link

Member

winoros Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's NewChunkWithCapacity

coocood reviewed Aug 7, 2018

View reviewed changes

jackysp reviewed Aug 8, 2018

View reviewed changes

coocood reviewed Aug 8, 2018

View reviewed changes

jackysp reviewed Aug 8, 2018

View reviewed changes

coocood reviewed Aug 8, 2018

View reviewed changes

jackysp reviewed Aug 8, 2018

View reviewed changes

zz-jason reviewed Aug 8, 2018

View reviewed changes

lysu mentioned this pull request Aug 9, 2018

miss "subquery returns more than 1 row" check when MaxChunkSize=1 #7333

Closed

pingcap deleted a comment from coocood Aug 9, 2018

lysu removed the status/all tests passed label Aug 9, 2018

lysu commented Aug 20, 2018

View reviewed changes

lysu commented Aug 21, 2018

View reviewed changes

coocood reviewed Aug 21, 2018

View reviewed changes

lysu added the status/all tests passed label Aug 21, 2018

lysu added 3 commits August 23, 2018 11:30

*:reduce chunk creation

4bfffe0

*:simplify renew method and remove useless newSize method

db64150

*:renew also take care old varlen column

cc671ae

lysu mentioned this pull request Aug 23, 2018

chunk: support capacity grow #7473

Merged

lysu closed this Sep 27, 2018

lysu deleted the dev/reduce_chunk_alloc branch September 27, 2018 04:12

lysu restored the dev/reduce_chunk_alloc branch September 27, 2018 04:12

		NewChunk() *chunk.Chunk

		// NewChunk creates a new chunk with given capacity.

executor: reduce chunk capacity #7141

executor: reduce chunk capacity #7141

Conversation

lysu commented Jul 24, 2018 • edited Loading

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

lysu commented Aug 2, 2018

winoros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Aug 6, 2018 • edited by shenli Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhexuany commented Aug 7, 2018

Choose a reason for hiding this comment

winoros commented Aug 7, 2018

lysu commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

lysu commented Aug 9, 2018

lysu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Aug 21, 2018

winoros commented Aug 21, 2018

lysu commented Aug 21, 2018

lysu commented Sep 27, 2018

lysu commented Jul 24, 2018 •

edited

Loading

lysu Aug 3, 2018 •

edited

Loading

lysu commented Aug 6, 2018 •

edited by shenli

Loading

jackysp Aug 8, 2018 •

edited

Loading

lysu Aug 8, 2018 •

edited

Loading