Skip to content

planner: implement the BatchPointGetPlan to improve the BatchPointGet performance#12322

Merged
sre-bot merged 18 commits intopingcap:masterfrom
lonng:batch-point-get-plan
Oct 10, 2019
Merged

planner: implement the BatchPointGetPlan to improve the BatchPointGet performance#12322
sre-bot merged 18 commits intopingcap:masterfrom
lonng:batch-point-get-plan

Conversation

@lonng
Copy link
Contributor

@lonng lonng commented Sep 23, 2019 •
edited
Loading

Signed-off-by: Lonng heng@lonng.org

What problem does this PR solve?

The SELECT * FROM t WEHRE (a, b) in ((1,2), (2,3)) will be transformed BatchPointGetExec to improve performance if the (a, b) is an unique index (#11750). But reuse the UnionAll physical plan, to archive this goal is not an efficiency implementation because it will call tryPointGet many times.

What is changed and how it works?

This PR implements an individual plan BatchPointGetPlan to improve this situation.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

  • Increased code complexity

Release note

  • implement the BatchPointGetPlan to improve the BatchPointGet performance

@lonng lonng added the sig/planner SIG: Planner label Sep 23, 2019
@ngaut
Copy link
Member

ngaut commented Sep 24, 2019

/bench

@lonng
Copy link
Contributor Author

lonng commented Sep 24, 2019 •
edited
Loading

@ngaut This PR just improve the BatchPointGet scenario. There is a snippet to bench it (https://gist.github.com/lonng/713bbcdd76393e868337706acd296352).

The following benchmark test on my local laptop.

colCount 1 inCount 1000 min 16.499829ms max 19.33945ms avg 17.313918ms
colCount 3 inCount 1000 min 25.087457ms max 38.9738ms avg 27.313907ms
colCount 5 inCount 1000 min 26.11047ms max 44.161137ms avg 28.937149ms
colCount 7 inCount 1000 min 28.639669ms max 40.138505ms avg 30.627987ms
colCount 9 inCount 1000 min 31.937538ms max 76.128493ms avg 38.582911ms

--- ↓master----
colCount 1 inCount 1000 min 22.34519ms max 45.719098ms avg 25.521175ms
colCount 3 inCount 1000 min 26.74264ms max 37.68117ms avg 28.132399ms
colCount 5 inCount 1000 min 28.94259ms max 42.281218ms avg 31.26947ms
colCount 7 inCount 1000 min 33.37555ms max 91.286487ms avg 42.735901ms
colCount 9 inCount 1000 min 37.927871ms max 76.951181ms avg 48.65512ms

image

@coocood
Copy link
Member

coocood commented Sep 24, 2019

@lonng
The benchmark is run on mocktikv which has the most cost.
We should run the benchmark on TiKV.

@lonng
Copy link
Contributor Author

lonng commented Sep 24, 2019 •
edited
Loading

@coocood
The following flame graph is tested in the local cluster (The TryFastPlan cost is reduced).

  • Master
    image
    image

  • This PR
    image
    image

@sre-bot
Copy link
Contributor

sre-bot commented Sep 24, 2019 

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 46113838b5b97a8cdb1fdaa4a6ef44ffdff2ec36
+++ tidb: e5681701c31102fe34533a2e3feec5c99ea0a208
tikv: 6ecea793538ee260b26e84ee3370807ecd591d21
pd: 5c648dc365e0025c4b7c4544bc2c593fe5c76c0b
================================================================================
test-1: < oltp_read_write >
    * QPS : 37173.59 ± 0.5584% (std=158.52) delta: 0.65%
    * AvgMs : 138.19 ± 0.4950% (std=0.54) delta: -0.63%
    * PercentileMs99 : 269.35 ± 1.0826% (std=2.38) delta: 0.73%
            
test-2: < oltp_point_select >
    * QPS : 80815.46 ± 2.0336% (std=932.52) delta: 0.73%
    * AvgMs : 3.16 ± 2.0860% (std=0.04) delta: -0.13%
    * PercentileMs99 : 6.77 ± 2.1283% (std=0.09) delta: 1.44%
            
test-3: < oltp_insert >
    * QPS : 21765.89 ± 1.1936% (std=187.45) delta: -0.49%
    * AvgMs : 11.76 ± 1.2249% (std=0.10) delta: 0.50%
    * PercentileMs99 : 24.38 ± 0.0000% (std=0.00) delta: 2.90%
            
test-4: < oltp_update_index >
    * QPS : 17031.11 ± 0.7484% (std=105.37) delta: -0.48%
    * AvgMs : 14.89 ± 0.3628% (std=0.04) delta: 0.24%
    * PercentileMs99 : 31.03 ± 1.0827% (std=0.27) delta: 2.56%
            
test-5: < oltp_update_non_index >
    * QPS : 29148.38 ± 0.4211% (std=97.11) delta: 0.11%
    * AvgMs : 8.78 ± 0.4556% (std=0.03) delta: -0.11%
    * PercentileMs99 : 19.09 ± 1.0688% (std=0.17) delta: 2.56%

https://perf.pingcap.com

@codecov
Copy link

codecov bot commented Sep 24, 2019 •
edited
Loading

Codecov Report

Merging #12322 into master will decrease coverage by 0.047%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #12322        +/-   ##
================================================
- Coverage   80.0317%   79.9846%   -0.0471%     
================================================
  Files           460        462         +2     
  Lines        103915     103171       -744     
================================================
- Hits          83165      82521       -644     
- Misses        14714      14758        +44     
+ Partials       6036       5892       -144

winoros
winoros reviewed Sep 24, 2019
View reviewed changes
@lonng lonng requested a review from coocood September 24, 2019 06:39
@lonng
Copy link
Contributor Author

lonng commented Sep 24, 2019

/run-all-tests

alivxxx
alivxxx reviewed Sep 24, 2019
View reviewed changes
winoros
winoros reviewed Sep 25, 2019
View reviewed changes
eurekaka
eurekaka reviewed Sep 25, 2019
View reviewed changes
planner/core/point_get_plan.go Outdated
Copy link
Contributor

@eurekaka eurekaka Sep 25, 2019

Choose a reason for hiding this comment

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

Shall we use physicalSchemaProducer instead of baseSchemaProducer to avoid unnecessary function overload such as StatsCount() below?

Copy link
Contributor Author

@lonng lonng Sep 25, 2019

Choose a reason for hiding this comment

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

The physicalSchemaProducer contains some unnecessary fields.

Copy link
Contributor

@eurekaka eurekaka Sep 26, 2019

Choose a reason for hiding this comment

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

But BatchPointGetPlan is actually a physical plan indeed?

Copy link
Contributor Author

@lonng lonng Sep 26, 2019

Choose a reason for hiding this comment

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

To make the BatchPointGetPlan struct size small, prefer to keep the current implementation (the same as PointGetPlan).

@lonng
planner: implement the BatchPointGetPlan to improve the BatchPointGet…
b2dce8a 
… performance

Signed-off-by: Lonng <heng@lonng.org>
@lonng
fix the SELECT * FROM t WHERE (key) in ((1), (2))
edbe9ae 
Signed-off-by: Lonng <heng@lonng.org>
@lonng
remove debug log
2071997 
Signed-off-by: Lonng <heng@lonng.org>
@lonng
address comment
8c24e78 
Signed-off-by: Lonng <heng@lonng.org>
@lonng
address comment
83e912f 
Signed-off-by: Lonng <heng@lonng.org>
@lonng
address comment
fc25444 
Signed-off-by: Lonng <heng@lonng.org>
@lonng
Copy link
Contributor Author

lonng commented Sep 25, 2019

/run-all-tests

alivxxx
alivxxx reviewed Sep 26, 2019
View reviewed changes
alivxxx
alivxxx reviewed Sep 26, 2019
View reviewed changes
@sre-bot sre-bot mentioned this pull request Sep 28, 2019
Merged
@lonng
Copy link
Contributor Author

lonng commented Sep 30, 2019

@eurekaka @lamxTyler PTAL again.

alivxxx
alivxxx reviewed Oct 10, 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

eurekaka
eurekaka approved these changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Oct 10, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 10, 2019

/run-all-tests

@sre-bot sre-bot merged commit c0d6185 into pingcap:master Oct 10, 2019
@lonng lonng deleted the batch-point-get-plan branch October 10, 2019 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@winoros winoros winoros left review comments

@eurekaka eurekaka eurekaka approved these changes

@coocood coocood Awaiting requested review from coocood

+1 more reviewer

@alivxxx alivxxx alivxxx left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. 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

@lonng @ngaut @coocood @sre-bot @eurekaka @winoros @alivxxx