Skip to content

planner: increase default concurrency factor of cost computing#11752

Merged
sre-bot merged 3 commits into
pingcap:masterfrom
eurekaka:cost/concurrency
Aug 16, 2019
Merged

planner: increase default concurrency factor of cost computing#11752
sre-bot merged 3 commits into
pingcap:masterfrom
eurekaka:cost/concurrency

Conversation

@eurekaka
Copy link
Copy Markdown
Contributor

@eurekaka eurekaka commented Aug 15, 2019

What problem does this PR solve?

#10581 incurs 2.19% performance regression of oltp_read_write workload, this PR resolves this regression.

What is changed and how it works?

The performance regression is caused by the fact that: oltp_read_write contains a class of aggregation quires without Group By, the new cost model would prefer HashAgg which would run in parallel, while the old cost model prefers single-threaded StreamAgg. According to the actual benchmark, single-threaded StreamAgg is a little bit faster than multi-threaded HashAgg for this kind of query.

This PR adjust default concurrency factor to make planner prefer StreamAgg for this kind of query.

Check List

Tests

  • Unit test

Code changes

N/A

Side effects

N/A

Related changes

N/A

@eurekaka eurekaka added type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Aug 15, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 15, 2019

Codecov Report

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

@@               Coverage Diff               @@
##            master     #11752        +/-   ##
===============================================
- Coverage   81.776%   81.5477%   -0.2284%     
===============================================
  Files          434        433         -1     
  Lines        94107      93506       -601     
===============================================
- Hits         76957      76252       -705     
- Misses       11684      11838       +154     
+ Partials      5466       5416        -50

@eurekaka
Copy link
Copy Markdown
Contributor Author

/run-all-tests

@eurekaka eurekaka marked this pull request as ready for review August 16, 2019 02:49
IndexReader_11 1104.45 root index:Selection_10
└─Selection_10 1104.45 cop lt(test.access_path_selection.b, 3)
└─IndexScan_9 3323.33 cop table:access_path_selection, index:a, b, range:[-inf,3), keep order:false, stats:pseudo
CREATE TABLE `outdated_statistics` (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why remove it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because this case is intended to test that planner would prefer idx_ab over idx_a, but it is hard to say whether double read using idx_ab is better than table scan or not. With the new concurrency factor, planner would prefer table scan, because there are only 6 rows in total.

Copy link
Copy Markdown
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM.

@lzmhhh123 lzmhhh123 added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 16, 2019
Copy link
Copy Markdown
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

@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 16, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 16, 2019

/run-all-tests

@sre-bot sre-bot merged commit 2009741 into pingcap:master Aug 16, 2019
@eurekaka eurekaka deleted the cost/concurrency branch August 16, 2019 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. type/bugfix This PR fixes a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants