*: fix memory leak after dom.Close #28027

lance6716 · 2021-09-14T03:08:52Z

What problem does this PR solve?

Issue Number: close #18048

Problem Summary: memory leak after dom.Close

What is changed and how it works?

How it Works:

remove store from domap when dom.Close
expose a way to not RegisterStatistics to global variables

Check List

Tests

Unit test

Side effects

Documentation

Release note

None

ti-chi-bot · 2021-09-14T03:08:54Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

gozssky
wjhuang2016

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.

lance6716 · 2021-09-14T03:09:58Z

/cc @wjhuang2016 @gozssky @glorv

sleepymole · 2021-09-14T03:20:19Z

domain/domain.go

@@ -605,6 +605,9 @@ func (do *Domain) isClose() bool {
 	return false
 }

+// DomainMapDelete is used to avoid cycle import with `session` package.
+var DomainMapDelete func (kv.Storage)


How about adding a `callback' to Domain instead of another global variable?

how can we pass the callback? domain package can't access session package

like this

type Domain struct { callback func() }

On the other hand, domain is created by domap.Get(), so is domap's responsibility to call domain.Close?

sleepymole · 2021-09-14T03:22:27Z

session/tidb.go

@@ -42,6 +42,12 @@ import (
 	"go.uber.org/zap"
 )

+func init() {
+	domain.DomainMapDelete = func(store kv.Storage) {
+		domap.Delete(store)


There is another global map storeBootstrapped which hold a storeUUID -> bool. ResetStoreForWithTiKVTest is a similar function to clean the two maps.

sleepymole · 2021-09-14T03:26:26Z

sessionctx/variable/statusvar.go

+var RegisterStatistics = registerStatistics
+
+// registerStatistics registers statistics.
+func registerStatistics(s Statistics) {


I think we should add a function like UnregisterStatistics and call it when domain or ddl is closed. It seems memory leak also happen in TiDB server, because BootstrapSession opens domain twice.

there's no id for Statistics, we can't find the one we appended (maybe by memory address?)

Directly comparing interface?

sleepymole · 2021-09-14T05:17:56Z

tidb/bindinfo/handle.go

Lines 106 to 122 in c26038b

    
           func NewBindHandle(ctx sessionctx.Context) *BindHandle { 
        
           	handle := &BindHandle{} 
        
           	handle.sctx.Context = ctx 
        
           	handle.bindInfo.Value.Store(make(cache, 32)) 
        
           	handle.bindInfo.parser = parser.New() 
        
           	handle.invalidBindRecordMap.Value.Store(make(map[string]*bindRecordUpdate)) 
        
           	handle.invalidBindRecordMap.flushFunc = func(record *BindRecord) error { 
        
           		return handle.DropBindRecord(record.OriginalSQL, record.Db, &record.Bindings[0]) 
        
           	} 
        
           	handle.pendingVerifyBindRecordMap.Value.Store(make(map[string]*bindRecordUpdate)) 
        
           	handle.pendingVerifyBindRecordMap.flushFunc = func(record *BindRecord) error { 
        
           		// BindSQL has already been validated when coming here, so we use nil sctx parameter. 
        
           		return handle.AddBindRecord(nil, record) 
        
           	} 
        
           	variable.RegisterStatistics(handle) 
        
           	return handle 
        
           }

BindHandle also need to unregister statistics.

lance6716 · 2021-09-14T05:20:59Z

tidb/bindinfo/handle.go

Lines 106 to 122 in c26038b

func NewBindHandle(ctx sessionctx.Context) *BindHandle {

handle := &BindHandle{}

handle.sctx.Context = ctx

handle.bindInfo.Value.Store(make(cache, 32))

handle.bindInfo.parser = parser.New()

handle.invalidBindRecordMap.Value.Store(make(map[string]*bindRecordUpdate))

handle.invalidBindRecordMap.flushFunc = func(record *BindRecord) error {

return handle.DropBindRecord(record.OriginalSQL, record.Db, &record.Bindings[0])

}

handle.pendingVerifyBindRecordMap.Value.Store(make(map[string]*bindRecordUpdate))

handle.pendingVerifyBindRecordMap.flushFunc = func(record *BindRecord) error {

// BindSQL has already been validated when coming here, so we use nil sctx parameter.

return handle.AddBindRecord(nil, record)

}

variable.RegisterStatistics(handle)

return handle

}

BindHandle also need to unregister statistics.

its assigned to Domain and unregistered in variable.UnregisterStatistics(do.bindHandle) when Domain.Close

sleepymole · 2021-09-14T05:23:30Z

tidb/bindinfo/handle.go

Lines 106 to 122 in c26038b

func NewBindHandle(ctx sessionctx.Context) *BindHandle {

handle := &BindHandle{}

handle.sctx.Context = ctx

handle.bindInfo.Value.Store(make(cache, 32))

handle.bindInfo.parser = parser.New()

handle.invalidBindRecordMap.Value.Store(make(map[string]*bindRecordUpdate))

handle.invalidBindRecordMap.flushFunc = func(record *BindRecord) error {

return handle.DropBindRecord(record.OriginalSQL, record.Db, &record.Bindings[0])

}

handle.pendingVerifyBindRecordMap.Value.Store(make(map[string]*bindRecordUpdate))

handle.pendingVerifyBindRecordMap.flushFunc = func(record *BindRecord) error {

// BindSQL has already been validated when coming here, so we use nil sctx parameter.

return handle.AddBindRecord(nil, record)

}

variable.RegisterStatistics(handle)

return handle

}

BindHandle also need to unregister statistics.

its assigned to Domain and unregistered in variable.UnregisterStatistics(do.bindHandle)

I missed it 😂

tiancaiamao · 2021-09-14T05:57:43Z

[2021-09-14T05:31:45.434Z] ----------------------------------------------------------------------
[2021-09-14T05:31:45.434Z] FAIL: bootstrap_test.go:324: testBootstrapSuite.TestIssue17979_2
[2021-09-14T05:31:45.434Z] 
[2021-09-14T05:31:45.434Z] bootstrap_test.go:364:
[2021-09-14T05:31:45.434Z]     c.Assert(config.GetGlobalConfig().OOMAction, Equals, config.OOMActionCancel)
[2021-09-14T05:31:45.434Z] ... obtained string = "log"
[2021-09-14T05:31:45.434Z] ... expected string = "cancel"
[2021-09-14T05:31:45.434Z] 
[2021-09-14T05:31:46.002Z] 
[2021-09-14T05:31:46.002Z] ----------------------------------------------------------------------

lance6716 · 2021-09-14T06:08:13Z

[2021-09-14T05:31:45.434Z] ----------------------------------------------------------------------
[2021-09-14T05:31:45.434Z] FAIL: bootstrap_test.go:324: testBootstrapSuite.TestIssue17979_2
[2021-09-14T05:31:45.434Z] 
[2021-09-14T05:31:45.434Z] bootstrap_test.go:364:
[2021-09-14T05:31:45.434Z]     c.Assert(config.GetGlobalConfig().OOMAction, Equals, config.OOMActionCancel)
[2021-09-14T05:31:45.434Z] ... obtained string = "log"
[2021-09-14T05:31:45.434Z] ... expected string = "cancel"
[2021-09-14T05:31:45.434Z] 
[2021-09-14T05:31:46.002Z] 
[2021-09-14T05:31:46.002Z] ----------------------------------------------------------------------

I can't reproduce it in my local go test -check.f "...TestIssue17979_2", need your help 😂

lance6716 · 2021-09-14T06:37:42Z

/merge

ti-chi-bot · 2021-09-14T06:37:45Z

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

Commit hash: 3e9e3c9

ti-chi-bot · 2021-09-14T06:37:54Z

@lance6716: 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.

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.

*: fix memory leak after dom.Close

c92aebd

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 14, 2021

Merge branch 'master' into tidb-as-library

58f8ce1

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 14, 2021

ti-chi-bot requested review from glorv, sleepymole and wjhuang2016 September 14, 2021 03:09

sleepymole reviewed Sep 14, 2021

View reviewed changes

lance6716 added 2 commits September 14, 2021 11:53

make check

0c225c6

address comment

361c7e9

try fix test

f9e0dee

sleepymole approved these changes Sep 14, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 14, 2021

wjhuang2016 approved these changes Sep 14, 2021

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 Sep 14, 2021

Merge branch 'master' into tidb-as-library

8f6ac2a

try fix test

3e9e3c9

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 14, 2021

Merge branch 'master' into tidb-as-library

39bfe93

Merge branch 'master' into tidb-as-library

0040a93

ti-chi-bot merged commit 310fef1 into pingcap:master Sep 14, 2021

lance6716 mentioned this pull request Sep 14, 2021

dep: update tidb to fix memory leak pingcap/dm#2133

Merged

w169q169 mentioned this pull request Sep 15, 2021

bindinfo: migrate test-infra to testify(staging-1) #27868

Merged

ti-chi-bot mentioned this pull request Sep 15, 2021

dep: update tidb to fix memory leak (#2133) pingcap/dm#2138

Merged

lance6716 mentioned this pull request Sep 17, 2021

test: fix unstable TestMemoryLeak #28111

Merged

12 tasks

lance6716 mentioned this pull request May 5, 2022

another implementation of schema tracker pingcap/tiflow#5334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: fix memory leak after dom.Close #28027

*: fix memory leak after dom.Close #28027

lance6716 commented Sep 14, 2021

ti-chi-bot commented Sep 14, 2021 •

edited

Loading

lance6716 commented Sep 14, 2021

sleepymole Sep 14, 2021

lance6716 Sep 14, 2021

sleepymole Sep 14, 2021

sleepymole Sep 14, 2021 •

edited

Loading

sleepymole Sep 14, 2021

sleepymole Sep 14, 2021 •

edited

Loading

lance6716 Sep 14, 2021

sleepymole Sep 14, 2021

sleepymole commented Sep 14, 2021

lance6716 commented Sep 14, 2021 •

edited

Loading

sleepymole commented Sep 14, 2021

tiancaiamao commented Sep 14, 2021

lance6716 commented Sep 14, 2021

lance6716 commented Sep 14, 2021

ti-chi-bot commented Sep 14, 2021

ti-chi-bot commented Sep 14, 2021

*: fix memory leak after dom.Close #28027

*: fix memory leak after dom.Close #28027

Conversation

lance6716 commented Sep 14, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 14, 2021 • edited Loading

lance6716 commented Sep 14, 2021

sleepymole Sep 14, 2021

Choose a reason for hiding this comment

lance6716 Sep 14, 2021

Choose a reason for hiding this comment

sleepymole Sep 14, 2021

Choose a reason for hiding this comment

sleepymole Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

sleepymole Sep 14, 2021

Choose a reason for hiding this comment

sleepymole Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

lance6716 Sep 14, 2021

Choose a reason for hiding this comment

sleepymole Sep 14, 2021

Choose a reason for hiding this comment

sleepymole commented Sep 14, 2021

lance6716 commented Sep 14, 2021 • edited Loading

sleepymole commented Sep 14, 2021

tiancaiamao commented Sep 14, 2021

lance6716 commented Sep 14, 2021

lance6716 commented Sep 14, 2021

ti-chi-bot commented Sep 14, 2021

ti-chi-bot commented Sep 14, 2021

ti-chi-bot commented Sep 14, 2021 •

edited

Loading

sleepymole Sep 14, 2021 •

edited

Loading

sleepymole Sep 14, 2021 •

edited

Loading

lance6716 commented Sep 14, 2021 •

edited

Loading