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

domain: fix memory leak for stats #7864

Merged
merged 3 commits into from Oct 11, 2018

Conversation

Projects
None yet
5 participants
@lamxTyler
Copy link
Member

lamxTyler commented Oct 10, 2018

What problem does this PR solve?

When the stats-lease is 0 or stats updating worker panics, the memory usage of stats will increase as long as there are new sessions.

What is changed and how it works?

When there is a new session, we will create a session stats collector, and it will be freed by the background stats worker when the session exits. Now only when the stats worker exists, which means the stats lease is not zero and the stats worker is running, we will create a new session stats collector.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    • Set the stats lease to 0
    • Create new session and exit repeatedly, the memory usage of stats do not increase.

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

PTAL @zz-jason @winoros @eurekaka

@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@@ -1066,7 +1066,7 @@ func CreateSession(store kv.Storage) (Session, error) {
privilege.BindPrivilegeManager(s, pm)

// Add statsUpdateHandle.
if do.StatsHandle() != nil {
if do.StatsHandle() != nil && do.StatsUpdating() {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Oct 10, 2018

Member

maybe it's better to add a comment to specify that the data collected by statsCollector is consumed by the background stats worker which periodically update stats using the collected data.

@winoros
Copy link
Member

winoros left a comment

this fix lgtm
i wonder whether we need to run a new goroutine do the updating when it's panicked?

@winoros
Copy link
Member

winoros left a comment

this fix lgtm
i wonder whether we need to run a new goroutine do the updating when it's panicked?

@lamxTyler

This comment has been minimized.

Copy link
Member Author

lamxTyler commented Oct 10, 2018

@winoros If we use a new goroutine, it may still panic.

@eurekaka
Copy link
Contributor

eurekaka left a comment

LGTM.

BTW, seems the existing SessionStatsCollectors would not be freed when the statsWorker panic. I guess it is because existing sessions may still write into this SessionStatsCollector. The problem is that these memory would never get chance to be freed then?

Maybe the session should check if it has SessionStatsCollector allocated while the statsWorker has gone, if so, the session should be responsible for freeing the memory when exiting. Anyway, that should be addressed in another issue I guess.

@shenli

This comment has been minimized.

Copy link
Member

shenli commented Oct 10, 2018

/run-all-tests

@lamxTyler lamxTyler merged commit db151bb into pingcap:master Oct 11, 2018

11 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@lamxTyler lamxTyler deleted the lamxTyler:stats-leak branch Oct 11, 2018

lamxTyler added a commit to lamxTyler/tidb that referenced this pull request Oct 11, 2018

lamxTyler added a commit to lamxTyler/tidb that referenced this pull request Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.