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.

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

RFC + executor: Support global memory tracker #16777

Merged
merged 46 commits into from
May 13, 2020

Conversation

Yisaer
Copy link
Contributor

@Yisaer Yisaer commented Apr 23, 2020

What problem does this PR solve?

UCP: #15407
Issue Number: close #15407

Support Global Memory tracker to control the memory usage and kill the current session's if the global memory usage in tracker exceeds the limit.

What is changed and how it works?

Currently, we have already support the Global Disk Tracker for Executor And the Support of Global memory Tracker for Executor is quite likely the same.

Proposal

see #17103

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Release note

  • Support Global Memory tracker for tidb-server

@Yisaer Yisaer requested a review from a team as a code owner April 23, 2020 15:11
@ghost ghost requested review from wshwsh12 and removed request for a team April 23, 2020 15:11
@Yisaer Yisaer marked this pull request as draft April 23, 2020 15:11
executor/executor.go Outdated Show resolved Hide resolved
executor/executor.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the sig/execution SIG execution label Apr 23, 2020
@Yisaer Yisaer changed the title [Draft] Global Mem tracker [Draft] executor: add global memory tracker Apr 23, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

Thanks for your contribution. If your PR get merged, you will be rewarded 3488 points.

@zz-jason zz-jason requested a review from eurekaka April 24, 2020 03:23
@Yisaer
Copy link
Contributor Author

Yisaer commented May 12, 2020

/run-all-tests

@Yisaer
Copy link
Contributor Author

Yisaer commented May 12, 2020

/run-unit-test

Comment on lines +343 to +344
t.parMu.Lock()
defer t.parMu.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we lock the parent in functions like Consume, instead, to lock it here?

Copy link
Contributor Author

@Yisaer Yisaer May 13, 2020

Choose a reason for hiding this comment

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

Lock parent is my first trial when I found the data race error, maybe adding lock in the Consume function is also enough. But the tpcc test result already seems ok if we lock parent here.

@Yisaer Yisaer requested a review from lzmhhh123 May 13, 2020 07:19
Copy link
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/LGT2 Indicates that a PR has LGTM 2. label May 13, 2020
@wshwsh12 wshwsh12 removed their request for review May 13, 2020 07:37
@lzmhhh123 lzmhhh123 added the status/can-merge Indicates a PR has been approved by a committer. label May 13, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 13, 2020

/run-all-tests

@sre-bot sre-bot merged commit 3f2d35a into pingcap:master May 13, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 13, 2020

Team Yisaer complete task #15407 and get 3488 score, current score 4038

@SunRunAway
Copy link
Contributor

@Yisaer Would you like to add documentation related to this PR?

@Yisaer
Copy link
Contributor Author

Yisaer commented May 13, 2020

@SunRunAway ok.

@sre-bot
Copy link
Contributor

sre-bot commented May 13, 2020

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 3f2d35a60bbc37382d991ecbcd48ebec9b6a3f8c
+++ tidb: 12596f1a74b6a6c4079423536817a57c4b853c7d
tikv: 58e5b3a1f39bf6473b06ed41c3371b64c7c374be
pd: 0e3d21884c952afe96af96b8c4f0ef851d5a9204
================================================================================
oltp_update_index:
    * QPS: 4449.87 ± 3.83% (std=104.04) delta: 1.51% (p=0.593)
    * Latency p50: 28.78 ± 3.94% (std=0.69) delta: -1.59%
    * Latency p99: 54.62 ± 5.95% (std=2.04) delta: -3.13%
            
oltp_insert:
    * QPS: 7102.58 ± 0.41% (std=17.68) delta: -0.06% (p=0.860)
    * Latency p50: 17.99 ± 0.13% (std=0.02) delta: -0.08%
    * Latency p99: 31.09 ± 0.90% (std=0.28) delta: -0.04%
            
oltp_read_write:
    * QPS: 14392.08 ± 0.03% (std=3.79) delta: -0.16% (p=0.842)
    * Latency p50: 178.17 ± 0.13% (std=0.15) delta: 0.12%
    * Latency p99: 331.91 ± 0.00% (std=0.00) delta: -0.58%
            
oltp_point_select:
    * QPS: 41406.07 ± 0.08% (std=25.29) delta: 3.20% (p=0.143)
    * Latency p50: 3.09 ± 0.00% (std=0.00) delta: -3.13%
    * Latency p99: 10.60 ± 2.24% (std=0.16) delta: -1.78%
            
oltp_update_non_index:
    * QPS: 4732.56 ± 6.86% (std=201.32) delta: -3.38% (p=0.619)
    * Latency p50: 27.09 ± 7.18% (std=1.19) delta: 3.68%
    * Latency p99: 41.35 ± 1.21% (std=0.35) delta: 0.10%
            

@Yisaer
Copy link
Contributor Author

Yisaer commented Sep 8, 2020

/label needs-cherry-pick-4.0

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 8, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #19861

@zz-jason
Copy link
Member

zz-jason commented Sep 8, 2020

@Yisaer @SunRunAway Is there any QA test to cover this new change?

@zz-jason
Copy link
Member

zz-jason commented Sep 8, 2020

BTW, please add a user document for it.

SunRunAway added a commit to SunRunAway/community that referenced this pull request Nov 26, 2020
Since @Yisaer has contributed a lot to sig-exec, according to sig-exec [roles-and-organization-management.md#promotion](https://github.com/pingcap/community/blob/master/special-interest-groups/sig-exec/roles-and-organization-management.md#from-reviewer-to-committer), we would like to promote him to our new committer, and his detailed contribution is shown below:

1. contribute [20+](https://github.com/pingcap/tidb/pulls?q=is%3Apr+author%3Ayisaer+label%3Astatus%2Fcan-merge+label%3Asig%2Fexecution) PRs to the executor,
2. support the [global memory tracker](pingcap/tidb#16777) (a medium task)
4. support the ratelimit action to solve the [table reader oom problem](pingcap/tidb#16104) (a hard task)
5. help other contributors and review [25+](https://github.com/pingcap/tidb/pulls?q=is%3Apr+reviewed-by%3Ayisaer+-author%3Ayisaer) PRs.

Thanks for his contribution!!
SunRunAway added a commit to SunRunAway/community that referenced this pull request Nov 27, 2020
Since @Yisaer has contributed a lot to sig-exec, according to sig-exec [roles-and-organization-management.md#promotion](https://github.com/pingcap/community/blob/master/special-interest-groups/sig-exec/roles-and-organization-management.md#from-reviewer-to-committer), we would like to promote him to our new committer, and his detailed contribution is shown below:

1. contribute [20+](https://github.com/pingcap/tidb/pulls?q=is%3Apr+author%3Ayisaer+label%3Astatus%2Fcan-merge+label%3Asig%2Fexecution) PRs to the executor,
2. support the [global memory tracker](pingcap/tidb#16777) (a medium task)
4. support the ratelimit action to solve the [table reader oom problem](pingcap/tidb#16104) (a hard task)
5. help other contributors and review [25+](https://github.com/pingcap/tidb/pulls?q=is%3Apr+reviewed-by%3Ayisaer+-author%3Ayisaer) PRs.

Thanks for his contribution!!
ti-chi-bot pushed a commit to pingcap/community that referenced this pull request Nov 30, 2020
Since @Yisaer has contributed a lot to sig-exec, according to sig-exec [roles-and-organization-management.md#promotion](https://github.com/pingcap/community/blob/master/special-interest-groups/sig-exec/roles-and-organization-management.md#from-reviewer-to-committer), we would like to promote him to our new committer, and his detailed contribution is shown below:

1. contribute [20+](https://github.com/pingcap/tidb/pulls?q=is%3Apr+author%3Ayisaer+label%3Astatus%2Fcan-merge+label%3Asig%2Fexecution) PRs to the executor,
2. support the [global memory tracker](pingcap/tidb#16777) (a medium task)
4. support the ratelimit action to solve the [table reader oom problem](pingcap/tidb#16104) (a hard task)
5. help other contributors and review [25+](https://github.com/pingcap/tidb/pulls?q=is%3Apr+reviewed-by%3Ayisaer+-author%3Ayisaer) PRs.

Thanks for his contribution!!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/config sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UCP: make memory usage of executors fit global memory limitation
7 participants