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.

Sign up for GitHub

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

Jump to bottom

executor: control Chunk size for Selection&Projection #9398

Merged
merged 10 commits into from
Feb 26, 2019
Merged

executor: control Chunk size for Selection&Projection #9398

merged 10 commits into from
Feb 26, 2019

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Feb 21, 2019 •
edited
Loading

What problem does this PR solve?

Control the number of rows in chunks returned by Selection & Projection.
Following up #9364, this PR is a subtask of #9166.

What is changed and how it works?

  • Make Selection support chunk size control.
  • Make Projection support chunk size control.
  • Add unit tests.

Check List

Tests

  • Unit test

@qw4990 qw4990 added the sig/execution SIG execution label Feb 21, 2019
@qw4990 qw4990 requested review from lysu and zz-jason February 21, 2019 06:45
@zz-jason zz-jason changed the title make Selection and Projection support chunk size control executormake Selection and Projection support chunk size control Feb 21, 2019
@zz-jason zz-jason changed the title executormake Selection and Projection support chunk size control executor: control Chunk size for Selection&Projection Feb 21, 2019
winoros
winoros reviewed Feb 21, 2019
View reviewed changes
executor/projection.go
@@ -166,6 +166,8 @@ func (e *ProjectionExec) isUnparallelExec() bool {
}

func (e *ProjectionExec) unParallelExecute(ctx context.Context, chk *chunk.Chunk) error {
// transmit the requiredRows
e.childResult.SetRequiredRows(chk.RequiredRows(), e.maxChunkSize)
Copy link
Member

Choose a reason for hiding this comment

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

Selection's childResult needs the same change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not?
It makes the child return as quickly as possible and also guarantees enough rows to return to Projection's parent.

@codecov-io
Copy link

codecov-io commented Feb 21, 2019 •
edited
Loading

Codecov Report

Merging #9398 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9398      +/-   ##
==========================================
+ Coverage    67.3%   67.31%   +0.01%     
==========================================
  Files         373      373              
  Lines       78442    78448       +6     
==========================================
+ Hits        52796    52809      +13     
+ Misses      20909    20905       -4     
+ Partials     4737     4734       -3
Impacted Files Coverage Δ
executor/projection.go 76.76% <100%> (+1.02%) ⬆️
executor/executor.go 68.16% <100%> (+0.27%) ⬆️
util/systimemon/systime_mon.go 80% <0%> (-20%) ⬇️
executor/index_lookup_join.go 77.42% <0%> (-0.63%) ⬇️
executor/join.go 79.58% <0%> (+0.51%) ⬆️
ddl/delete_range.go 79.36% <0%> (+4.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9afbda9...5303a60. Read the comment docs.

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 21, 2019 •
edited
Loading

Projection becomes a pipeline breaker when it in parallel mode, and according to its implementation, it's hard to support chunk size control in this mode, so I only make it work in unparallel mode.

Selection only supports chunk size control upward to its parent now.
While calling the child's Next, it can't determine the RequiredRows because the selectivity is unknown.
If we have a estimated selectivity, maybe we can set RequiredRows of child to (RequiredRows from parent) / selectivity?

How do you think?

@lysu
Copy link
Contributor

lysu commented Feb 21, 2019

Projection becomes a pipeline breaker when it in parallel mode, and according to its implementation, it's useless to support chunk size control in this mode, so I only make it work in unparallel mode.

but in parallel mode, projection's child executor still get benefits from known "Require Not More than this info"?

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 21, 2019

Projection becomes a pipeline breaker when it in parallel mode, and according to its implementation, it's useless to support chunk size control in this mode, so I only make it work in unparallel mode.

but in parallel mode, projection's child executor still get benefits from known "Require Not More than this info"?

Yes, but in parallel mode, all worker goroutines work in the background and Projection fetches data from its child asynchronously, so it's hard to determine the value of RequiredRows passed to child.

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 22, 2019

Projection in parallel mode supports chunk size control now.
I introduce requiredRowsNow into Projection and update it when each call of Next.
Then the background fetcher uses this value as its own chunks' RequiredRows while fetching data from child.

PTAL~ @lysu

zz-jason
zz-jason reviewed Feb 22, 2019
View reviewed changes
executor/projection.go Outdated Show resolved Hide resolved
executor/projection.go Outdated Show resolved Hide resolved
@qw4990 qw4990 force-pushed the chunkSizeControl_Sel/Proj branch from 9249589 to f67fa59 Compare February 24, 2019 12:51
alivxxx
alivxxx reviewed Feb 25, 2019
View reviewed changes
executor/projection.go Outdated Show resolved Hide resolved
zz-jason
zz-jason reviewed Feb 25, 2019
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

/run-all-tests

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 25, 2019
@zz-jason zz-jason requested a review from alivxxx February 25, 2019 09:05
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 25, 2019

/run-all-tests

@qw4990 qw4990 mentioned this pull request Feb 25, 2019
Merged
@qw4990 qw4990 force-pushed the chunkSizeControl_Sel/Proj branch from 725cb81 to 102f98c Compare February 25, 2019 12:41
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 25, 2019

/run-all-tests

@qw4990 qw4990 force-pushed the chunkSizeControl_Sel/Proj branch from 102f98c to 47e0f8e Compare February 25, 2019 12:53
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 25, 2019

/run-all-tests

@qw4990 qw4990 force-pushed the chunkSizeControl_Sel/Proj branch from 47e0f8e to 5294f93 Compare February 26, 2019 02:39
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 26, 2019

/run-all-tests

alivxxx
alivxxx previously approved these changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 26, 2019
@alivxxx alivxxx dismissed their stale review February 26, 2019 02:47

Unit test failed

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 26, 2019

/run-all-tests

1 similar comment
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 26, 2019

/run-all-tests

@cwen0
Copy link
Member

cwen0 commented Feb 26, 2019

/rebuild

@cwen0
Copy link
Member

cwen0 commented Feb 26, 2019

/run-unit-test

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 26, 2019

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 26, 2019

/run-all-tests

alivxxx
alivxxx approved these changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 merged commit 3e0f51b into pingcap:master Feb 26, 2019
@qw4990 qw4990 mentioned this pull request Apr 10, 2019
Merged
qw4990 added a commit to qw4990/tidb that referenced this pull request Apr 11, 2019
@qw4990
executor: control Chunk size for Selection&Projection (pingcap#9398)
ccd9b51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

@winoros winoros winoros left review comments

@alivxxx alivxxx alivxxx approved these changes

@lysu lysu Awaiting requested review from lysu

Assignees
No one assigned
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@qw4990 @codecov-io @lysu @zz-jason @cwen0 @winoros @alivxxx