Revise default background threads size by Lloyd-Pottiger · Pull Request #4723 · pingcap/tiflash

Lloyd-Pottiger · 2022-04-20T10:46:15Z

Signed-off-by: Lloyd-Pottiger yan1579196623@gmail.com

What problem does this PR solve?

Issue Number: close #4702

Problem Summary:

What is changed and how it works?

set the size of background & blockable background thread pool and GRPCCompletionQueuePool to a quarter of server_info.cpu_info.logical_cores.
Add two functions to separate initialization and get of BackgroundProcessingPool.

BackgroundProcessingPool & Context::initializeBackgroundPool(UInt16 pool_size);
BackgroundProcessingPool & Context::initializeBlockableBackgroundPool(UInt16 pool_size);

add initialization of background & blockable background thread pool in TiFlashTestEnv.

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

Release note

None

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot · 2022-04-20T10:46:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
JinheLin

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.

Details

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

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JaySon-Huang · 2022-04-20T10:59:47Z

better to use the function getNumberOfPhysicalCPUCores

Lloyd-Pottiger · 2022-04-20T11:43:17Z

better to use the function getNumberOfPhysicalCPUCores

Maybe it is better to use logical cpu numbers rather than physical cpu number? Usually one physical cpu have more than one cpu core, and there are hyper-threading in most cpus.

Lloyd-Pottiger · 2022-04-21T00:24:11Z

/run-all-tests

sre-bot · 2022-04-21T00:54:23Z

Coverage for changed files

Filename                                                                               Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Files which contain no functions:
/home/jenkins/agent/workspace/tiflash-build-common/tiflash/dbms/src/Core/Defines.h           0                 0         -           0                 0         -           0                 0         -           0                 0         -
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                        0                 0         -           0                 0         -           0                 0         -           0                 0         -

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
17132      9492             44.59%    192860  96270        50.08%

full coverage report (for internal network access only)

Lloyd-Pottiger · 2022-04-21T02:55:47Z

/cc @JinheLin @JaySon-Huang

JinheLin · 2022-04-21T04:07:57Z

better to use the function getNumberOfPhysicalCPUCores

Maybe it is better to use logical cpu numbers rather than physical cpu number? Usually one physical cpu have more than one cpu core, and there are hyper-threading in most cpus.

Also, I think physical cpu is not a good idea in cloud environment.

JaySon-Huang · 2022-04-21T04:13:07Z

Also, I think physical cpu is not a good idea in cloud environment.

We planned to make getNumberOfPhysicalCPUCores respect the k8s/docker CPU limit, which is more friendly for cloud env https://github.com/pingcap/tiflash/pull/3852/files

But that PR is blocked since now @jiaqizho has no spare time to get that addressed and merged. Maybe @Lloyd-Pottiger take over that PR and continue to get it merged?

JinheLin · 2022-04-21T04:27:47Z

Also, I think physical cpu is not a good idea in cloud environment.

We planned to make getNumberOfPhysicalCPUCores respect the k8s/docker CPU limit, which is more friendly for cloud env https://github.com/pingcap/tiflash/pull/3852/files

But that PR is blocked since now @jiaqizho has no spare time to get that addressed and merged. Maybe @Lloyd-Pottiger take over that PR and continue to get it merged?

The CPU limit of cgroup is CPU time and it is related to logical CPU. Physical CPU is not suitable too.

I think @Lloyd-Pottiger can look into this PR and try to get it done.

Lloyd-Pottiger · 2022-04-21T04:39:28Z

Also, I think physical cpu is not a good idea in cloud environment.

We planned to make getNumberOfPhysicalCPUCores respect the k8s/docker CPU limit, which is more friendly for cloud env https://github.com/pingcap/tiflash/pull/3852/files

But that PR is blocked since now @jiaqizho has no spare time to get that addressed and merged. Maybe @Lloyd-Pottiger take over that PR and continue to get it merged?

@JaySon-Huang @JinheLin OK, I will

jiaqizho · 2022-04-21T04:46:56Z

Also, I think physical cpu is not a good idea in cloud environment.

We planned to make getNumberOfPhysicalCPUCores respect the k8s/docker CPU limit, which is more friendly for cloud env https://github.com/pingcap/tiflash/pull/3852/files
But that PR is blocked since now @jiaqizho has no spare time to get that addressed and merged. Maybe @Lloyd-Pottiger take over that PR and continue to get it merged?

@JaySon-Huang @JinheLin OK, I will

https://github.com/pingcap/tiflash/pull/3852/files a tip: This PR tested on cgroup. but have not been tested in k8s/docker.

Lloyd-Pottiger · 2022-04-21T04:50:48Z

Also, I think physical cpu is not a good idea in cloud environment.

We planned to make getNumberOfPhysicalCPUCores respect the k8s/docker CPU limit, which is more friendly for cloud env https://github.com/pingcap/tiflash/pull/3852/files
But that PR is blocked since now @jiaqizho has no spare time to get that addressed and merged. Maybe @Lloyd-Pottiger take over that PR and continue to get it merged?

@JaySon-Huang @JinheLin OK, I will

https://github.com/pingcap/tiflash/pull/3852/files a tip: This PR tested on cgroup. but have not been tested in k8s/docker.

thx, I will try

… cores Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger · 2022-06-10T09:53:42Z

/run-all-tests

sre-bot · 2022-06-10T10:21:51Z

Coverage for changed files

Filename                                Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Interpreters/Context.cpp                    518               329    36.49%         168                90    46.43%        1138               705    38.05%         276               209    24.28%
Interpreters/Context.h                       13                 5    61.54%          13                 5    61.54%          13                 5    61.54%           0                 0         -
Storages/BackgroundProcessingPool.h           2                 0   100.00%           2                 0   100.00%           2                 0   100.00%           0                 0         -

Files which contain no functions:
Core/Defines.h                                0                 0         -           0                 0         -           0                 0         -           0                 0         -
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                       533               334    37.34%         183                95    48.09%        1153               710    38.42%         276               209    24.28%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18330      9659             47.30%    205690  96734        52.97%

full coverage report (for internal network access only)

JaySon-Huang · 2022-06-14T10:14:56Z

/run-all-tests

dbms/src/Server/Server.cpp

dbms/src/Interpreters/Settings.h

Lloyd-Pottiger · 2022-06-14T10:44:28Z

/run-all-tests

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Co-authored-by: JaySon <tshent@qq.com>

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger · 2022-06-15T00:54:47Z

/run-all-tests

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger · 2022-06-15T02:24:32Z

/run-all-tests

sre-bot · 2022-06-15T02:58:14Z

Coverage for changed files

Filename                                  Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Interpreters/Context.cpp                      514               323    37.16%         168                88    47.62%        1134               693    38.89%         272               207    23.90%
Interpreters/Context.h                         13                 5    61.54%          13                 5    61.54%          13                 5    61.54%           0                 0         -
Interpreters/Settings.h                         1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Storages/BackgroundProcessingPool.cpp          91                26    71.43%          10                 1    90.00%         182                27    85.16%          56                18    67.86%
Storages/BackgroundProcessingPool.h             2                 0   100.00%           2                 0   100.00%           2                 0   100.00%           0                 0         -
TestUtils/TiFlashTestEnv.cpp                   22                 3    86.36%           4                 0   100.00%          67                 9    86.57%          12                 5    58.33%

Files which contain no functions:
Core/Defines.h                                  0                 0         -           0                 0         -           0                 0         -           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                         643               357    44.48%         198                94    52.53%        1399               734    47.53%         340               230    32.35%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18330      9655             47.33%    205690  96694        52.99%

full coverage report (for internal network access only)

JaySon-Huang

LGTM

And please update the description of this PR

Lloyd-Pottiger · 2022-06-15T05:39:54Z

/merge

ti-chi-bot · 2022-06-15T05:39:55Z

@Lloyd-Pottiger: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-06-15T05:39:57Z

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

Details

Commit hash: e227778

ti-chi-bot · 2022-06-15T05:40:10Z

@Lloyd-Pottiger: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

sre-bot · 2022-06-15T06:08:48Z

Coverage for changed files

Filename                                  Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Interpreters/Context.cpp                      533               330    38.09%         169                88    47.93%        1155               700    39.39%         290               216    25.52%
Interpreters/Context.h                         13                 5    61.54%          13                 5    61.54%          13                 5    61.54%           0                 0         -
Interpreters/Settings.h                         1                 0   100.00%           1                 0   100.00%           1                 0   100.00%           0                 0         -
Storages/BackgroundProcessingPool.cpp          91                25    72.53%          10                 1    90.00%         182                26    85.71%          56                17    69.64%
Storages/BackgroundProcessingPool.h             2                 0   100.00%           2                 0   100.00%           2                 0   100.00%           0                 0         -
TestUtils/TiFlashTestEnv.cpp                   22                 3    86.36%           4                 0   100.00%          67                 9    86.57%          12                 5    58.33%

Files which contain no functions:
Core/Defines.h                                  0                 0         -           0                 0         -           0                 0         -           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                         662               363    45.17%         199                94    52.76%        1420               740    47.89%         358               238    33.52%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18327      9653             47.33%    205795  96644        53.04%

full coverage report (for internal network access only)

This reverts commit a79ad91.

revise default background threads size

4440967

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 20, 2022

format

bcb61ed

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot requested review from JaySon-Huang and JinheLin April 21, 2022 02:55

flowbehappy added the type/enhancement The issue or PR belongs to an enhancement. label Apr 21, 2022

Lloyd-Pottiger and others added 2 commits June 10, 2022 12:37

Merge branch 'master' into revise-default-background-pool-size

c762a1c

set the thread pool size to the a quarter of of number of logical CPU…

0a6134b

… cores Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 10, 2022

Lloyd-Pottiger added 2 commits June 10, 2022 16:47

format

96bb7a7

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

format include headers

2724e96

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger requested review from JinheLin and removed request for JaySon-Huang and JinheLin June 10, 2022 10:22

JaySon-Huang reviewed Jun 14, 2022

View reviewed changes

dbms/src/Server/Server.cpp Outdated Show resolved Hide resolved

JaySon-Huang reviewed Jun 14, 2022

View reviewed changes

dbms/src/Interpreters/Settings.h Outdated Show resolved Hide resolved

JaySon-Huang reviewed Jun 14, 2022

View reviewed changes

dbms/src/Interpreters/Settings.h Outdated Show resolved Hide resolved

Lloyd-Pottiger and others added 5 commits June 14, 2022 18:53

address comments

7b065e5

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

address comments

a4c3476

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Update dbms/src/Interpreters/Settings.h

77d6871

Co-authored-by: JaySon <tshent@qq.com>

revise some comments expression

4eea54d

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

revise some comments expression

9f59c92

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger added 2 commits June 15, 2022 10:08

initialize thread pool in TiFlashTestEnv

691fc6c

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

format

e227778

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JaySon-Huang approved these changes Jun 15, 2022

View reviewed changes

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 Jun 15, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 15, 2022

Merge branch 'master' into revise-default-background-pool-size

3c51508

ti-chi-bot merged commit a79ad91 into pingcap:master Jun 15, 2022

Lloyd-Pottiger deleted the revise-default-background-pool-size branch June 15, 2022 06:23

Lloyd-Pottiger added a commit that referenced this pull request Jun 21, 2022

Revert "Revise default background threads size (#4723)"

80999ba

This reverts commit a79ad91.

Lloyd-Pottiger mentioned this pull request Jun 21, 2022

Revert "Revise default background threads size" #5176

Merged

12 tasks

Lloyd-Pottiger added a commit to Lloyd-Pottiger/tiflash that referenced this pull request Jul 19, 2022

Revert "Revise default background threads size (pingcap#4723)"

a517a2b

This reverts commit a79ad91.

Conversation

Lloyd-Pottiger commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaySon-Huang commented Apr 20, 2022

Uh oh!

Lloyd-Pottiger commented Apr 20, 2022

Uh oh!

Lloyd-Pottiger commented Apr 21, 2022

Uh oh!

sre-bot commented Apr 21, 2022

Coverage for changed files

Coverage summary

Uh oh!

Lloyd-Pottiger commented Apr 21, 2022

Uh oh!

JinheLin commented Apr 21, 2022

Uh oh!

JaySon-Huang commented Apr 21, 2022

Uh oh!

JinheLin commented Apr 21, 2022

Uh oh!

Lloyd-Pottiger commented Apr 21, 2022

Uh oh!

jiaqizho commented Apr 21, 2022

Uh oh!

Lloyd-Pottiger commented Apr 21, 2022

Uh oh!

Lloyd-Pottiger commented Jun 10, 2022

Uh oh!

sre-bot commented Jun 10, 2022

Coverage for changed files

Coverage summary

Uh oh!

JaySon-Huang commented Jun 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lloyd-Pottiger commented Jun 14, 2022

Uh oh!

Lloyd-Pottiger commented Jun 15, 2022

Uh oh!

Lloyd-Pottiger commented Jun 15, 2022

Uh oh!

sre-bot commented Jun 15, 2022

Coverage for changed files

Coverage summary

Uh oh!

JaySon-Huang left a comment

Choose a reason for hiding this comment

Uh oh!

Lloyd-Pottiger commented Jun 15, 2022

Uh oh!

ti-chi-bot commented Jun 15, 2022

Uh oh!

ti-chi-bot commented Jun 15, 2022

Uh oh!

ti-chi-bot commented Jun 15, 2022

Uh oh!

sre-bot commented Jun 15, 2022

Coverage for changed files

Coverage summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Lloyd-Pottiger commented Apr 20, 2022 •

edited

Loading

ti-chi-bot commented Apr 20, 2022 •

edited

Loading