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

*: use pprof profile to collect CPU time group by SQL and plan digest #24892

Merged
merged 67 commits into from
Jun 2, 2021

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented May 25, 2021

Track issue: #24875

What problem does this PR solve?

This PR uses pprof profile labels to collect CPU time group by SQL and plan digest.

What is changed and how it works?

The implementation is:

1: Use SQL digest and plan digest as the pprof labels.

// After parse and calculated SQL digest, set the sql digest label
ctx = pprof.WithLabels(ctx, pprof.Labels("sql_digest", sql_digest))
pprof.SetGoroutineLabels(ctx)

...

// After the optimizer generated plan, set the plan digest label.
ctx = pprof.WithLabels(ctx, pprof.Labels("plan_digest", plan_digest))
pprof.SetGoroutineLabels(ctx)

There is a special case, for execute stmt SQL, should use the prepare SQL digest as the sql_digest pprof label.

  1. Start 2 background worker to fetch profile and analyze the profile data.

image

Using a buffer channel to avoid parse and output take too much time that infect the fetching profile work.

Other problem

Because the profile worker keep calling pprof.StartCPUProfile to fetch profile data, other place (such pprof profile HTTP API handler) call pprof.StartCPUProfile will be failed.

In order to solve this problem, other place should fetch profile data from profile worker instead of pprof.StartCPUProfile.

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • No release note

crazycs520 and others added 19 commits May 19, 2021 10:22
Signed-off-by: crazycs <[email protected]>
Signed-off-by: crazycs <[email protected]>
Signed-off-by: crazycs <[email protected]>
Signed-off-by: crazycs <[email protected]>
Signed-off-by: crazycs <[email protected]>
Signed-off-by: crazycs <[email protected]>
Signed-off-by: crazycs <[email protected]>
Signed-off-by: crazycs <[email protected]>
@crazycs520 crazycs520 added type/usability size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 25, 2021
@crazycs520 crazycs520 self-assigned this May 25, 2021
@crazycs520 crazycs520 requested a review from a team as a code owner May 25, 2021 13:58
@crazycs520 crazycs520 requested review from wshwsh12 and removed request for a team May 25, 2021 13:58
@crazycs520 crazycs520 mentioned this pull request May 25, 2021
7 tasks
@crazycs520
Copy link
Contributor Author

/run-all-tests

@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels May 25, 2021
Signed-off-by: crazycs <[email protected]>
executor/executor.go Outdated Show resolved Hide resolved
@crazycs520
Copy link
Contributor Author

/run-all-tests

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests

@breezewish
Copy link
Member

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-common-test
/run-integration-common-test
/run-unit-test

@crazycs520
Copy link
Contributor Author

/run-common-test
/run-integration-common-test

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-common-test
/run-integration-common-test

@crazycs520
Copy link
Contributor Author

/run-common-test

@crazycs520
Copy link
Contributor Author

/run-common-test tidb-test=master

@ti-srebot
Copy link
Contributor

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/build

@ti-srebot
Copy link
Contributor

@crazycs520
Copy link
Contributor Author

/run-check_release_note

@crazycs520
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 75e8d94

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 2, 2021
@crazycs520
Copy link
Contributor Author

/merge

@ti-chi-bot ti-chi-bot merged commit 7811bf9 into pingcap:master Jun 2, 2021
@Linda-wang00
Copy link

Linda-wang00 commented Jul 3, 2021 via email

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 sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra 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. type/usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants