executor: fix the issue that `pb` memory cannot be released quickly #10815

qw4990 · 2019-06-14T11:08:25Z

What problem does this PR solve?

Fix the issue that pb memory cannot be released quickly.

What is changed and how it works?

In this PR(stmtctx.go:296), we let sc hold detail until the query finish to get more coptask's execution information.

Because detail is nested in copResponse, if we hold its pointer, the Golang GC scanner will regard copResponse as active, and will not release until the query finish.

Then another field pbResp held by copResonse will not be released quickly.

Since pbResp is used to buffer pb data, it results in more memory consumption.

Check List

Tests

qw4990 · 2019-06-14T11:11:46Z

We need more tests for this case.
Do you have any suggestions about how to test this case?
@zz-jason @XuHuaiyu @lysu

codecov · 2019-06-14T11:13:12Z

Codecov Report

Merging #10815 into master will increase coverage by 0.0074%.
The diff coverage is 53.8461%.

@@               Coverage Diff                @@
##             master     #10815        +/-   ##
================================================
+ Coverage   80.2889%   80.2964%   +0.0074%     
================================================
  Files           417        417                
  Lines         88453      88456         +3     
================================================
+ Hits          71018      71027         +9     
+ Misses        12231      12221        -10     
- Partials       5204       5208         +4

codecov · 2019-06-14T11:13:12Z

Codecov Report

Merging #10815 into master will increase coverage by 0.0211%.
The diff coverage is 53.8461%.

@@               Coverage Diff               @@
##            master     #10815        +/-   ##
===============================================
+ Coverage   80.829%   80.8502%   +0.0211%     
===============================================
  Files          419        417         -2     
  Lines        88608      88262       -346     
===============================================
- Hits         71621      71360       -261     
+ Misses       11759      11693        -66     
+ Partials      5228       5209        -19

zz-jason · 2019-06-14T12:10:05Z

We need more tests for this case.
Do you have any suggestions about how to test this case?
@zz-jason @XuHuaiyu @lysu

Can we check the two numbers of runtime.MemStats.HeapObjects before and after calling runtime.GC()?

qw4990 · 2019-06-14T12:22:43Z

We need more tests for this case.
Do you have any suggestions about how to test this case?
@zz-jason @XuHuaiyu @lysu

Can we check the two numbers of runtime.MemStats.HeapObjects before and after calling runtime.GC()?

There are many unit tests running parallelly, so they may influence each other to let this unit test unstable.

crazycs520 · 2019-06-14T12:56:10Z

/run-all-tests

lysu · 2019-06-14T12:59:38Z

emm..parallel running is a problem...

maybe we can make an inuse assert util...

the idea is that after running just fetch a heap profile, using runtime.MemProfile, just stealing code from

https://github.com/golang/go/blob/cfbe3cfbeb6b2de561fc709b8644d4d7a4e182bb/src/runtime/pprof/pprof.go#L533

then just loop []runtime.MemProfileRecord

https://github.com/golang/go/blob/f9a4ae018d99c5afb0e4f128545ff26e01d7b498/src/runtime/pprof/protomem.go#L38

then if we can find any MemProfileRecord that:

stack contains current running test method name e.g. TestDistSqlLeak
locate line meet some condition?
and inuse > 0?

lysu · 2019-06-14T13:01:44Z

....but this case held memory is alloc in another goroutine...stack isn't start with TestDistSqlLeak 🤣

qw4990 · 2019-06-17T03:22:57Z

After discussing with @XuHuaiyu and reviewing check.RunAll, we can use SerialSuites to let our UT be run serially.
Using SerialSuites and runtime.GC() can meet the test requirement.

qw4990 · 2019-06-17T13:23:14Z

/run-all-tests

qw4990 · 2019-06-17T13:24:29Z

A unit test has been added, PTAL~ @XuHuaiyu @lysu @zz-jason

XuHuaiyu · 2019-06-18T02:36:11Z

executor/memory_test.go

+	// prepare data
+	totalSize := uint64(256 << 20) // 256MB
+	blockSize := uint64(8 << 10)   // 8KB
+	delta := totalSize / 5


What does this mean?

Just an empirical value, used to judge if memory leak happens.
The difference between InUseHeap before and after a query must be less than it.

zz-jason · 2019-06-18T03:14:18Z

executor/memory_test.go

+	}
+	d, err := session.BootstrapSession(s.store)
+	c.Assert(err, IsNil)
+	d.SetStatsUpdating(true)


no need to perform?

The unnecessary code in testMemoryLeak has been removed now.

XuHuaiyu · 2019-06-18T03:33:53Z

executor/memory_test.go

+	se.Close()
+	runtime.GC()
+	allocatedFinal, inUseFinal := s.readMem()
+	c.Assert(allocatedFinal-allocatedAfter, Less, delta)


Why do we check allocatedFinal-allocatedAfter here?

To ensure that these executors don't allocate much memory when closing.

XuHuaiyu · 2019-06-18T06:54:23Z

executor/memory_test.go

+	runtime.GC()
+	allocatedAfter, inUseAfter := s.readMem()
+	c.Assert(allocatedAfter-allocatedBegin, GreaterEqual, totalSize)
+	c.Assert(s.memDiff(inUseAfter, inUseBegin), Less, delta)


Do we need to check s.memDiff(inUseAfter, inUseBegin) before line 86?

This check ensures that the memory used in this query can be recycled by the GC scanner.
If we put this check before line 86, since we don't know if GC happens when processing this query, the value of s.memDiff(inUseAfter, inUseBegin) would be indeterminate.

zz-jason

LGTM

XuHuaiyu

LGTM

…ingcap#10815)

…10815) (#10833)

…10815) (#10832)

qw4990 added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Jun 14, 2019

qw4990 requested review from lysu, zz-jason and XuHuaiyu June 14, 2019 11:08

qw4990 force-pushed the fix_mem branch 2 times, most recently from 2837656 to 4b356bf Compare June 17, 2019 13:15

qw4990 added the status/all tests passed label Jun 17, 2019

XuHuaiyu reviewed Jun 18, 2019

View reviewed changes

zz-jason reviewed Jun 18, 2019

View reviewed changes

qw4990 added 5 commits June 18, 2019 11:30

fix issue that pb memory cannot be released quickly

ff5eb27

WIP: add UT

b6f5b08

add UT

62425a5

polist UT

abf2a7e

refmt

9fe1baa

qw4990 force-pushed the fix_mem branch from 3e4e2b2 to 9fe1baa Compare June 18, 2019 03:31

XuHuaiyu reviewed Jun 18, 2019

View reviewed changes

remove some unnecessary code

8b48d20

XuHuaiyu reviewed Jun 18, 2019

View reviewed changes

zz-jason added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Jun 18, 2019

zz-jason reviewed Jun 18, 2019

View reviewed changes

Merge branch 'master' into fix_mem

aad5420

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 18, 2019

XuHuaiyu approved these changes Jun 18, 2019

View reviewed changes

Merge branch 'master' into fix_mem

992a6d6

qw4990 merged commit beb8268 into pingcap:master Jun 18, 2019

qw4990 added a commit to qw4990/tidb that referenced this pull request Jun 18, 2019

executor: fix the issue that pb memory cannot be released quickly (p…

b69967b

…ingcap#10815)

qw4990 mentioned this pull request Jun 18, 2019

executor: fix the issue that pb memory cannot be released quickly (#10815) #10832

Merged

qw4990 added a commit to qw4990/tidb that referenced this pull request Jun 18, 2019

executor: fix the issue that pb memory cannot be released quickly (p…

9c4ae05

…ingcap#10815)

qw4990 mentioned this pull request Jun 18, 2019

executor: fix the issue that pb memory cannot be released quickly (#10815) #10833

Merged

zz-jason pushed a commit that referenced this pull request Jun 19, 2019

executor: fix the issue that pb memory cannot be released quickly (#…

f185720

…10815) (#10833)

zz-jason pushed a commit that referenced this pull request Jun 19, 2019

executor: fix the issue that pb memory cannot be released quickly (#…

076e3b2

…10815) (#10832)

csuzhangxc mentioned this pull request Aug 22, 2019

Use mydumper to back up a database containing many tables, very slow #11733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: fix the issue that `pb` memory cannot be released quickly #10815

executor: fix the issue that `pb` memory cannot be released quickly #10815

qw4990 commented Jun 14, 2019 •

edited

Loading

qw4990 commented Jun 14, 2019

codecov bot commented Jun 14, 2019

codecov bot commented Jun 14, 2019 •

edited

Loading

zz-jason commented Jun 14, 2019

qw4990 commented Jun 14, 2019

crazycs520 commented Jun 14, 2019

lysu commented Jun 14, 2019

lysu commented Jun 14, 2019

qw4990 commented Jun 17, 2019

qw4990 commented Jun 17, 2019

qw4990 commented Jun 17, 2019

XuHuaiyu Jun 18, 2019

qw4990 Jun 18, 2019

zz-jason Jun 18, 2019

qw4990 Jun 18, 2019 •

edited

Loading

XuHuaiyu Jun 18, 2019

qw4990 Jun 18, 2019

XuHuaiyu Jun 18, 2019

qw4990 Jun 18, 2019 •

edited

Loading

zz-jason left a comment

XuHuaiyu left a comment

executor: fix the issue that pb memory cannot be released quickly #10815

executor: fix the issue that pb memory cannot be released quickly #10815

Conversation

qw4990 commented Jun 14, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

qw4990 commented Jun 14, 2019

codecov bot commented Jun 14, 2019

Codecov Report

codecov bot commented Jun 14, 2019 • edited Loading

Codecov Report

zz-jason commented Jun 14, 2019

qw4990 commented Jun 14, 2019

crazycs520 commented Jun 14, 2019

lysu commented Jun 14, 2019

lysu commented Jun 14, 2019

qw4990 commented Jun 17, 2019

qw4990 commented Jun 17, 2019

qw4990 commented Jun 17, 2019

XuHuaiyu Jun 18, 2019

Choose a reason for hiding this comment

qw4990 Jun 18, 2019

Choose a reason for hiding this comment

zz-jason Jun 18, 2019

Choose a reason for hiding this comment

qw4990 Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

XuHuaiyu Jun 18, 2019

Choose a reason for hiding this comment

qw4990 Jun 18, 2019

Choose a reason for hiding this comment

XuHuaiyu Jun 18, 2019

Choose a reason for hiding this comment

qw4990 Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

XuHuaiyu left a comment

Choose a reason for hiding this comment

executor: fix the issue that `pb` memory cannot be released quickly #10815

executor: fix the issue that `pb` memory cannot be released quickly #10815

qw4990 commented Jun 14, 2019 •

edited

Loading

codecov bot commented Jun 14, 2019 •

edited

Loading

qw4990 Jun 18, 2019 •

edited

Loading

qw4990 Jun 18, 2019 •

edited

Loading