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

*: use chunk grow for simple executor #7540

Merged
merged 13 commits into from Sep 27, 2018

Conversation

@lysu
Copy link
Member

lysu commented Aug 29, 2018

What problem does this PR solve?

ref issue #7077 and base on PR #7473

in this PR we start to make executor use chunk grow.

What is changed and how it works?

There are several small modification in this PR.

  • modify ChunkList use chunk grow, more bigger chunk node as more chunk node inserted
  • Prepare, Deallocate, Simple, Set, Insert, Dual, Sort, MaxOneRow, Update, Delete, TopN, Limit use NoDataCap for exec result chunk
  • CancelDDLJobsExec, ShowDDLJobQueriesExec, ShowDDLJobsExec, SelectionExec, TableScanExec, ExplainExec, ShowExec, UnionScanExec check batch size use chunk.Capacity() instead of baseExecutor.maxChunkSize
  • use GrowAndReset to replace Reset in some executors
  • replace some chunk.NewChunkWithCapacity(which will be depercated soon) with chunk.New

This PR doesn't modify join/agg/distsql/sort operators which are the most sophisticated. We will handle them in next PR.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Refactor

Side effects

  • No

Related changes

  • No for this PR.

This change is Reviewable

@lysu lysu force-pushed the lysu:dev-simple-executor-chunk branch 2 times, most recently from 4afec4d to a82186d Aug 29, 2018

@lysu

This comment has been minimized.

Copy link
Member Author

lysu commented Aug 29, 2018

/run-all-tests

@lysu lysu force-pushed the lysu:dev-simple-executor-chunk branch 2 times, most recently from a18bcce to 3475a7c Sep 3, 2018

@lysu lysu removed the status/WIP label Sep 3, 2018

@lysu lysu force-pushed the lysu:dev-simple-executor-chunk branch from 3475a7c to 29bede7 Sep 3, 2018

@@ -420,8 +421,9 @@ func (b *executorBuilder) buildLimit(v *plan.PhysicalLimit) Executor {
b.err = errors.Trace(b.err)
return nil
}
n := int(mathutil.MinUint64(v.Count, math.MaxInt64))

This comment has been minimized.

Copy link
@lamxTyler

lamxTyler Sep 5, 2018

Member

Why math.MaxInt64?

@@ -69,7 +69,7 @@ func (s *testSuite) TestSelectNormal(c *C) {
response.Fetch(context.TODO())

// Test Next.
chk := chunk.NewChunkWithCapacity(colTypes, 32)
chk := chunk.New(colTypes, 32, 32*3)

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 10, 2018

Member

why 32*3?

@@ -102,7 +103,7 @@ func (e *baseExecutor) Schema() *expression.Schema {

// newChunk creates a new chunk to buffer current executor's result.
func (e *baseExecutor) newChunk() *chunk.Chunk {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 10, 2018

Member

It seems that after this patch, newChunk should only be called once in the parent operator?

This comment has been minimized.

Copy link
@lysu

lysu Sep 10, 2018

Author Member

yes~

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 10, 2018

Member

how about renaming it?

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 26, 2018

Member

@lysu how about renaming it to newFirstChk()

@@ -133,6 +135,16 @@ func newBaseExecutor(ctx sessionctx.Context, schema *expression.Schema, id strin
return e
}

func (e baseExecutor) withInitCap(initCap int) (ret baseExecutor) {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 10, 2018

Member

how about:

  • s/withInitCap/setInitCap/
  • don't return baseExecutor, because this object takes a lot of memory

This comment has been minimized.

Copy link
@lysu

lysu Sep 10, 2018

Author Member

withXX and return is common chain builder pattern, I test that go compiler can inline but still do return alloc and copy as you said , so we back to ugly way 🤣

@@ -38,6 +38,7 @@ type Chunk struct {
// Capacity constants.
const (
InitialCapacity = 32
NoDataChunkCap = -1

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 10, 2018

Member

how about:

ZeroCapacity = 0

This comment has been minimized.

Copy link
@lysu

lysu Sep 10, 2018

Author Member

0 is ok, but it will still alloc columns and field num of column.. use magic -1 we can alloc less.

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 26, 2018

Member

considered there is a InitialCapacity, I think ZeroCapacity is more suitable.

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Sep 10, 2018

@XuHuaiyu PTAL

@@ -553,7 +553,7 @@ func (s *session) ExecRestrictedSQL(sctx sessionctx.Context, sql string) ([]chun
)
// Execute all recordset, take out the first one as result.
for i, rs := range recordSets {
tmp, err := drainRecordSet(ctx, rs)
tmp, err := drainRecordSet(ctx, sctx, rs)

This comment has been minimized.

Copy link
@lamxTyler

lamxTyler Sep 10, 2018

Member

Maybe it is better to use se here because it is the session that is used to execute the sql.

@lysu lysu force-pushed the lysu:dev-simple-executor-chunk branch from 64d0045 to db746bd Sep 10, 2018

e.initCap = initCap
}

func (e *baseExecutor) setMaxChunkSize(maxChunkSize int) {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 10, 2018

Member

can be removed?

@@ -133,6 +135,14 @@ func newBaseExecutor(ctx sessionctx.Context, schema *expression.Schema, id strin
return e
}

func (e *baseExecutor) setInitCap(initCap int) {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 10, 2018

Member

maybe these kind of setters can be removed?

  1. the function is not exported to be used as an API.
  2. the function logic is too simple.

@lysu lysu force-pushed the lysu:dev-simple-executor-chunk branch from f461b2b to bad3ffd Sep 12, 2018

@@ -121,6 +122,7 @@ func newBaseExecutor(ctx sessionctx.Context, schema *expression.Schema, id strin
ctx: ctx,
id: id,
schema: schema,
initCap: ctx.GetSessionVars().MaxChunkSize,

This comment has been minimized.

Copy link
@coocood

coocood Sep 14, 2018

Member

Should set to math.Min(chunk.InitialCapacity, ctx.GetSessionVars().MaxChunkSize)?

This comment has been minimized.

Copy link
@lysu

lysu Sep 25, 2018

Author Member

In this PR, we can not do that, because JOIN relate modification will in next PR which will make grow real work.

then we will make initCap be configurable and check constrait in set config stage.

@@ -296,6 +293,10 @@ func (p *MySQLPrivilege) loadTable(sctx sessionctx.Context, sql string,
return errors.Trace(err)
}
}
// NOTE: decodeTableRow decodes data from a chunk Row, that is a shallow copy.

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Sep 17, 2018

Contributor

Please read the NOTE once again! @lysu

This comment has been minimized.

Copy link
@lysu

lysu Sep 17, 2018

Author Member

yes, in here, use Renew will always create a new chunk just like old logic 😆

This comment has been minimized.

Copy link
@tiancaiamao

@lysu lysu force-pushed the lysu:dev-simple-executor-chunk branch from bad3ffd to d5919c5 Sep 21, 2018

@lysu lysu force-pushed the lysu:dev-simple-executor-chunk branch from d5919c5 to 4efa618 Sep 25, 2018

@lysu

This comment has been minimized.

Copy link
Member Author

lysu commented Sep 25, 2018

PTAL @XuHuaiyu

@lysu

This comment has been minimized.

Copy link
Member Author

lysu commented Sep 26, 2018

fping @XuHuaiyu

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

tiancaiamao commented Sep 26, 2018

LGTM

@@ -38,6 +38,7 @@ type Chunk struct {
// Capacity constants.
const (
InitialCapacity = 32
ZeroCapacity = -1

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 26, 2018

Member

ZeroCapacity = 0?

@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Sep 26, 2018

/run-all-tests

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Sep 26, 2018

@lysu

This comment has been minimized.

Copy link
Member Author

lysu commented Sep 26, 2018

/run-all-tests

@zz-jason zz-jason merged commit 05b37de into pingcap:master Sep 27, 2018

11 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@lysu lysu deleted the lysu:dev-simple-executor-chunk branch Sep 27, 2018

dbjoa added a commit to cloud-pi/tidb that referenced this pull request Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.