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

copr: increase extra concurrency for small coprocessor tasks #37725

Merged
merged 9 commits into from
Oct 14, 2022
Merged

copr: increase extra concurrency for small coprocessor tasks #37725

merged 9 commits into from
Oct 14, 2022

Conversation

you06
Copy link
Contributor

@you06 you06 commented Sep 8, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: close #37724

Problem Summary:

What is changed and how it works?

When there are many small coprocessor tasks, the default value of tidb_distsql_scan_concurrency harms the latency. This PR increases extra concurrency when we exactly know there are small coprocessor tasks.

I tested this PR with the script.

Test result with different tidb_distsql_scan_concurrency before this PR

tidb_distsql_scan_concurrency latency
1 1.40s
2 647ms
4 299ms
8 141ms
16 67.0ms
32 41.0ms
64 34.4ms
128 33.9ms
256 32.2ms

Tested with query shared into 1000 regions and got the following result(avg of 100 round test):

test with default tidb_distsql_scan_concurrency
before this PR: 70.2ms
after this PR:  33.2ms

Check List

Tests

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

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Optimize handles read requests by increasing extra concurrency for small coprocessor tasks.

@you06 you06 requested a review from a team as a code owner September 8, 2022 13:18
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 8, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • guo-shaoge
  • wshwsh12

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 8, 2022
@hawkingrei
Copy link
Member

/run-all-tests

@you06
Copy link
Contributor Author

you06 commented Sep 14, 2022

@wshwsh12 PTAL

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2022
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2022
@cfzjywxk
Copy link
Contributor

cfzjywxk commented Oct 9, 2022

@you06
Any update?

@you06
Copy link
Contributor Author

you06 commented Oct 9, 2022

@you06 Any update?

I'm fixing the tests broken by this PR.

@you06
wip
2de31c5 
Signed-off-by: you06 <you1474600@gmail.com>

for test

Signed-off-by: you06 <you1474600@gmail.com>

fix bug

Signed-off-by: you06 <you1474600@gmail.com>

new formula for concurrency & add test

Signed-off-by: you06 <you1474600@gmail.com>

fix comment

Signed-off-by: you06 <you1474600@gmail.com>

remove unused function

Signed-off-by: you06 <you1474600@gmail.com>

set private concurrency for small tasks

Signed-off-by: you06 <you1474600@gmail.com>

fix build

Signed-off-by: you06 <you1474600@gmail.com>

fix panic && add test

Signed-off-by: you06 <you1474600@gmail.com>

fix bug and test

Signed-off-by: you06 <you1474600@gmail.com>

fix tests

Signed-off-by: you06 <you1474600@gmail.com>

fix tests

Signed-off-by: you06 <you1474600@gmail.com>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 12, 2022
cfzjywxk
cfzjywxk reviewed Oct 12, 2022
View reviewed changes
store/copr/coprocessor.go Outdated Show resolved Hide resolved
store/copr/coprocessor.go Show resolved Hide resolved
store/copr/coprocessor_test.go Show resolved Hide resolved
@you06
fix bug & add test
944b4b5 
Signed-off-by: you06 <you1474600@gmail.com>
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 12, 2022
@you06
Merge branch 'master' into small-copr-opt
006edfb
@you06
update comment
fd73900 
Signed-off-by: you06 <you1474600@gmail.com>
@you06
fix test
4b4a9f5 
Signed-off-by: you06 <you1474600@gmail.com>
wshwsh12
wshwsh12 reviewed Oct 13, 2022
View reviewed changes
store/copr/coprocessor.go Show resolved Hide resolved
store/copr/coprocessor.go Outdated Show resolved Hide resolved
@you06 @wshwsh12
Update store/copr/coprocessor.go
c0bb7b8 
Co-authored-by: Shenghui Wu <wshwsh12@outlook.com>
wshwsh12
wshwsh12 approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 14, 2022
@cfzjywxk
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 51e4eac

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 14, 2022
@ti-chi-bot ti-chi-bot merged commit 61eed5c into pingcap:master Oct 14, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Oct 14, 2022

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci/integration-cdc-test 🟥 failed 1, success 36, total 37 26 min New failing
idc-jenkins-ci-tidb/integration-common-test 🔴 failed 3, success 14, total 17 13 min Existing failure
idc-jenkins-ci-tidb/tics-test 🔴 failed 1, success 0, total 1 4 min 34 sec Existing failure
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 23 min Existing passed
idc-jenkins-ci-tidb/common-test 🟢 all 11 tests passed 10 min Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 8 min 26 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 4 min 16 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 3 min 3 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 2 min 54 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

@zhangjinpeng87
Copy link
Contributor

Any bench result to show the great improvement of this change?

@you06
Copy link
Contributor Author

you06 commented Oct 14, 2022

Any bench result to show the great improvement of this change?

I attached the bench result to the description.

@zhangjinpeng87
Copy link
Contributor

Any bench result to show the great improvement of this change?

I attached the bench result to the description.

When there many requests has fan out characteristics, the test result including p99 should be much more better?

@cfzjywxk
Copy link
Contributor

cfzjywxk commented Oct 17, 2022 •
edited
Loading

Any bench result to show the great improvement of this change?

I attached the bench result to the description.

When there many requests has fan out characteristics, the test result including p99 should be much more better?

@zhangjinpeng1987
The average latency and p99 should both be better if the coprocessor tasks are not accumulated on the tikv side, and this PR just optimizes the task wait on the tidb side. If there's already task queueing on the tikv side, this change may increase latency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfzjywxk cfzjywxk cfzjywxk approved these changes

@guo-shaoge guo-shaoge guo-shaoge approved these changes

@wshwsh12 wshwsh12 wshwsh12 approved these changes

Assignees
No one assigned
Labels
release-note size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. 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.

Optimize handle read in large cluster
8 participants
@you06 @ti-chi-bot @hawkingrei @cfzjywxk @sre-bot @zhangjinpeng87 @guo-shaoge @wshwsh12