*: Support to trigger GC when memory usage is large. #38179

wshwsh12 · 2022-09-26T14:35:28Z

What problem does this PR solve?

Issue Number: ref #37816

Problem Summary:

What is changed and how it works?

Add the system variable tidb_server_memory_limit_gc_trigger and support to trigger GC when memory usgae is larger than tidb_server_memory_limit_gc_trigger * tidb_server_memory_limit.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2022-09-26T14:35:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

XuHuaiyu
hawkingrei

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.

XuHuaiyu · 2022-09-29T03:45:47Z

util/gctuner/memory_limit_tuner.go

+// So we can change memory limit dynamically to avoid frequent GC when memory usage is greater than the soft limit.
+type memoryLimitTuner struct {
+	finalizer    *finalizer
+	isTuning     atomic.Bool


Why not use atomicutil.Bool?

XuHuaiyu · 2022-10-10T03:32:24Z

util/gctuner/memory_limit_tuner.go

+	debug.SetMemoryLimit(softLimit)
+}
+
+func (t *memoryLimitTuner) calcSoftMemoryLimit() int64 {


Suggested change

func (t *memoryLimitTuner) calcSoftMemoryLimit() int64 {

func (t *memoryLimitTuner) calcMemoryLimit() int64 {

XuHuaiyu · 2022-10-10T03:36:37Z

util/gctuner/memory_limit_tuner.go

+	ratio := float64(100+gogc) / 100
+	// If theoretical NextGC(Equivalent to HeapInUse * (100 + GOGC) / 100) is bigger than MemoryLimit twice in a row,
+	// the second GC is caused by MemoryLimit.
+	if r.HeapInuse > uint64(float64(debug.SetMemoryLimit(-1))/ratio) {


Is r.HeapInuse * ratio more readable according to the comment above?

We need to explain why "twice". It really hard to understand

XuHuaiyu · 2022-10-10T06:22:11Z

util/gctuner/memory_limit_tuner_test.go

+
+	memory210mb := allocator.alloc(210 << 20)
+	require.True(t, gcNum < getNowGCNum())
+	// Test waiting for reset


Do we need to check whether waitingReset is true here?

XuHuaiyu · 2022-10-10T06:23:03Z

util/gctuner/memory_limit_tuner_test.go

+
+	allocator.free(memory210mb)
+	allocator.free(memory100mb)
+	// Can GC in 80% again


What does Can GC mean?

XuHuaiyu · 2022-10-10T06:28:01Z

util/gctuner/memory_limit_tuner_test.go

+	gcNum = getNowGCNum()
+	memory210mb = allocator.alloc(210 << 20)
+	time.Sleep(100 * time.Millisecond)
+	require.True(t, gcNum < getNowGCNum())


Does this GC trigger by memory_limit?

XuHuaiyu · 2022-10-10T06:28:57Z

util/gctuner/memory_limit_tuner_test.go

+	require.True(t, gcNum < getNowGCNum())
+	allocator.free(memory210mb)
+	allocator.free(memory600mb)
+	time.Sleep(1 * time.Second) // If test.count > 1, wait tuning finished.


Should we check isTunning, waitingReset and times after this Sleep?

XuHuaiyu · 2022-10-10T09:10:53Z

util/gctuner/memory_limit_tuner.go

+		if t.times >= 2 && t.waitingReset.CompareAndSwap(false, true) {
+			t.times = 0
+			go func() {
+				debug.SetMemoryLimit(math.MaxInt)


MaxInt or MaxInt64?

XuHuaiyu · 2022-10-11T03:28:04Z

util/gctuner/memory_limit_tuner.go

+	// If theoretical NextGC(Equivalent to HeapInUse * (100 + GOGC) / 100) is bigger than MemoryLimit twice in a row,
+	// the second GC is caused by MemoryLimit.
+	// All GC is divided into the following three cases:
+	// 1. In normal, HeapInUse * (100 + GOGC) / 100 < MemoryLimit, NextGC = HeapInUse * (100 + GOGC) / 100.
+	// 2. The first time HeapInUse * (100 + GOGC) / 100 >= MemoryLimit, NextGC = MemoryLimit. But this GC is trigger by GOGC.
+	// 3. The second time HeapInUse * (100 + GOGC) / 100 >= MemoryLimit. This GC is trigger by MemoryLimit.
+	//    We set MemoryLimit to MaxInt, so the NextGC will be HeapInUse * (100 + GOGC) / 100 again.


Suggested change

// If theoretical NextGC(Equivalent to HeapInUse * (100 + GOGC) / 100) is bigger than MemoryLimit twice in a row,

// the second GC is caused by MemoryLimit.

// All GC is divided into the following three cases:

// 1. In normal, HeapInUse * (100 + GOGC) / 100 < MemoryLimit, NextGC = HeapInUse * (100 + GOGC) / 100.

// 2. The first time HeapInUse * (100 + GOGC) / 100 >= MemoryLimit, NextGC = MemoryLimit. But this GC is trigger by GOGC.

// 3. The second time HeapInUse * (100 + GOGC) / 100 >= MemoryLimit. This GC is trigger by MemoryLimit.

// We set MemoryLimit to MaxInt, so the NextGC will be HeapInUse * (100 + GOGC) / 100 again.

This `if` checks whether the **last** GC was triggered by MemoryLimit as far as possible.

If the **last** GC was triggered by MemoryLimit, we'll set MemoryLimit to MAXVALUE to return control back to GOGC to avoid frequent GC when memory usage fluctuates above and below MemoryLimit.

The logic we judge whether the **last** GC was triggered by MemoryLimit is as follows:

suppose `NextGC` = `HeapInUse * (100 + GOGC) / 100)`,

- If NextGC < MemoryLimit, the **next** GC will **not** be triggered by MemoryLimit thus we do not care about why the **last** GC is triggered. And MemoryLimit will not be reset this time.

- Only if NextGC >= MemoryLimit , the **next** GC will be triggered by MemoryLimit. Thus we need to reset MemoryLimit after the next GC happens if needed.

XuHuaiyu · 2022-10-11T03:39:35Z

util/gctuner/memory_limit_tuner.go

+	memoryLimit := t.calcMemoryLimit()
+	if EnableGOGCTuner.Load() {
+		memoryLimit = math.MaxInt64
+	}


Suggested change

memoryLimit := t.calcMemoryLimit()

if EnableGOGCTuner.Load() {

memoryLimit = math.MaxInt64

}

memoryLimit := math.MaxInt64

if !EnableGOGCTuner.Load() {

memoryLimit = t.calcMemoryLimit()

}

XuHuaiyu · 2022-10-11T03:40:51Z

util/gctuner/memory_limit_tuner.go

+				debug.SetMemoryLimit(math.MaxInt)
+				resetInterval := 1 * time.Minute // Wait 1 minute and set back, to avoid frequent GC
+				failpoint.Inject("testMemoryLimitTuner", func(val failpoint.Value) {
+					if val, ok := val.(bool); val && ok {


why check ok ?

For linter, type assertion must be checked (forcetypeassert)

wshwsh12 · 2022-10-12T01:47:18Z

/merge

ti-chi-bot · 2022-10-12T01:47:21Z

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

Commit hash: 0c39f03

sre-bot · 2022-10-12T02:48:40Z

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test	🟥 failed 1, success 5, total 6	10 min	New failing
idc-jenkins-ci/integration-cdc-test	✅ all 37 tests passed	25 min	Fixed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	16 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	15 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	7 min 18 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	7 min 7 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 39 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 38 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 58 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

wshwsh12 added 6 commits September 26, 2022 11:02

finalizer

4b65e3c

temp

d2f00d2

Merge remote-tracking branch 'upstream/master' into auto-gc-80

9bfe357

temp2

ec3cf48

demo

2506a14

fix name

33dfdb0

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 26, 2022

wshwsh12 mentioned this pull request Sep 28, 2022

Support global memory control #37816

Closed

9 tasks

wshwsh12 marked this pull request as ready for review September 28, 2022 05:44

wshwsh12 requested a review from a team as a code owner September 28, 2022 05:44

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2022

wshwsh12 added 3 commits September 28, 2022 13:44

Merge branch 'master' into auto-gc-80

a2ed5f2

fix lint

5cb85ee

add ut

65fa37c

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 28, 2022

rename

77870f3

wshwsh12 force-pushed the auto-gc-80 branch from e67bc76 to 77870f3 Compare September 28, 2022 09:40

wshwsh12 added 3 commits September 28, 2022 18:09

fmt

01116aa

Merge branch 'master' into auto-gc-80

73b9678

add license

f47abc0

XuHuaiyu reviewed Oct 10, 2022

View reviewed changes

wshwsh12 added 3 commits October 10, 2022 14:05

address comments

4764c92

Merge remote-tracking branch 'upstream/master' into auto-gc-80

58fa721

clean code

035a1e7

XuHuaiyu reviewed Oct 10, 2022

View reviewed changes

address comments

7b8a72f

wshwsh12 added 2 commits October 10, 2022 16:03

clean

6cb6a26

fix lint

2ebcd0c

wshwsh12 requested a review from XuHuaiyu October 10, 2022 09:01

XuHuaiyu reviewed Oct 11, 2022

View reviewed changes

wshwsh12 added 2 commits October 11, 2022 11:46

address comments

2321b61

address comments

0c39f03

wshwsh12 requested review from XuHuaiyu and hawkingrei October 11, 2022 06:01

XuHuaiyu approved these changes Oct 11, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 11, 2022

hawkingrei approved these changes Oct 11, 2022

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 Oct 11, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 12, 2022

Merge branch 'master' into auto-gc-80

fd76ecb

ti-chi-bot merged commit 37d4357 into pingcap:master Oct 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Support to trigger GC when memory usage is large. #38179

*: Support to trigger GC when memory usage is large. #38179

wshwsh12 commented Sep 26, 2022 •

edited

ti-chi-bot commented Sep 26, 2022 •

edited

XuHuaiyu Sep 29, 2022

XuHuaiyu Oct 10, 2022

XuHuaiyu Oct 10, 2022

XuHuaiyu Oct 10, 2022

XuHuaiyu Oct 10, 2022

XuHuaiyu Oct 10, 2022

XuHuaiyu Oct 10, 2022

XuHuaiyu Oct 10, 2022

XuHuaiyu Oct 10, 2022

XuHuaiyu Oct 11, 2022

XuHuaiyu Oct 11, 2022

XuHuaiyu Oct 11, 2022

wshwsh12 Oct 11, 2022

wshwsh12 commented Oct 12, 2022

ti-chi-bot commented Oct 12, 2022

sre-bot commented Oct 12, 2022

	func (t *memoryLimitTuner) calcSoftMemoryLimit() int64 {
	func (t *memoryLimitTuner) calcMemoryLimit() int64 {

*: Support to trigger GC when memory usage is large. #38179

*: Support to trigger GC when memory usage is large. #38179

Conversation

wshwsh12 commented Sep 26, 2022 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wshwsh12 commented Oct 12, 2022

ti-chi-bot commented Oct 12, 2022

sre-bot commented Oct 12, 2022

TiDB MergeCI notify

wshwsh12 commented Sep 26, 2022 •

edited

ti-chi-bot commented Sep 26, 2022 •

edited