drainer/: Reduce memory usage (#735) #737

lichunzhu · 2019-09-02T11:33:50Z

What problem does this PR solve?

Cherry pick of #735.
TOOL-1480
Current drainer uses buffed channel to cache some instances which may cause OOM when facing binlogs which consumes too much memory.

What is changed and how it works?

Avoid any buffer at upstream to avoid too much memory usage and not decreasing performance.
Add txnManager in loader to manage cached txn memory usage. Without txnManager the performance of loader will be affected.

To check the effectiveness, we make two tests

In the following:
v1 means 097db42, which is the newest master branch.
v2 means 1e51514, which is the current branch.
In v1, maxBinlogItemCount=512
In v2, maxBinlogItemCount=0

TEST 1: Small binlogs, to validate the efficiency of drainer

I put the test of v1 and v2 in one picture. 15:30 ~ 15:50 is test of v1 while 15:53 ~ 16:13 is test of v2. As we can see the memory usage and drainer event of both branches are similar.
drainer event:

memory:

execution time:

TEST 2: the upstream data one binlog near 60M, to validate the momery usage of drainer

For v1 and situations we set loader.input,loader.success, dsyncer.success buffer to 1024 drainer will get an OOM problem. The memory usage will rise up to more than 100G.
For v2, the memory usage is around 2G. The drainer event for DDL is around 2k and is around 14k for DMLs.
drainer event:

memory:

execute time:

Check List

Tests

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

Code changes

Side effects

Possible performance regression

Related changes

Need to update the documentation
Need to update the tidb-ansible repository
Need to be included in the release note

…czli/drainer/reduceMemoryUsage

WangXiangUSTC · 2019-09-05T03:35:26Z

LGTM

drainer/pump_test.go

suzaku

LGTM

lichunzhu · 2019-09-06T02:42:38Z

DNM now. This PR should better be merged after release-2.1 version.

july2993 · 2019-09-08T09:34:46Z

do you run it at the case there lag ?
In v1, maxBinlogItemCount=512

for this, the mem will not take only less than 1G
60M * 512 = at least 30G

also you may need to change pkg/loader ' input buf size.
keep in mind what you need to do is prove there will not take too much memory when there's no lag.

…czli/drainer/reduceMemoryUsage

…ip binlog by previous test case

…czli/drainer/reduceMemoryUsage

…ichunzhu/tidb-binlog into czli/drainer/reduceMemoryUsage

IANTHEREAL · 2019-09-24T10:58:52Z

@suzaku PTAL

lichunzhu · 2019-09-25T02:11:37Z

DNM now, I will add some unit tests about txnManager.

IANTHEREAL · 2019-09-25T02:13:09Z

please complete it today @lichunzhu

… when block at cond.Wait() or input is closed. add one unit test to test whether txnManager can handle txn whose size if bigger than maxCacheSize

lichunzhu · 2019-09-25T04:53:41Z

/run-all-tests tidb=release-3.0 tikv=release-3.0 pd=release-3.0

lichunzhu · 2019-09-25T05:14:46Z

@GregoryIan PTAL

suzaku · 2019-09-25T08:28:39Z

pkg/loader/load_test.go

+		select {
+		case input <- txn:
+		case <-time.After(50 * time.Microsecond):
+			c.Fatal("txnManager gets blocked while receiving txns")


If the 5th txn get blocked sending to input, why didn't this test fail?

The 5th txn was picked from input but can't be added to the txnManager.cacheChan. Although it gets blocked at cond.Wait() but it's still picked out from input. By the way, we can't know a txn's size when it is not picked out from input.

The check c.Assert(output, check.HasLen, 4) can make sure it's not added.

pkg/loader/load_test.go

suzaku · 2019-09-25T08:30:13Z

pkg/loader/load_test.go

+	case t := <-output:
+		txnManager.pop(t)
+		c.Assert(t, check.DeepEquals, txn)
+	case <-time.After(50 * time.Microsecond):


No need to wait? Just use default, because if we can't get a txn at this point, something must be wrong.

This place default can be used.

suzaku · 2019-09-25T08:33:47Z

pkg/loader/load_test.go

+	output := txnManager.run()
+	select {
+	case input <- txnSmall:
+	case <-time.After(50 * time.Microsecond):


Use default?

In txnManager.run() it will start a new goroutine and start watching input channel which will take some time. Using default may get fault.

lichunzhu · 2019-09-25T09:22:41Z

/run-unit-tests

suzaku · 2019-09-25T09:26:35Z

pkg/loader/load_test.go

@@ -413,13 +413,13 @@ func (s *txnManagerSuite) TestRunTxnManager(c *check.C) {
 	case t := <-output:
 		txnManager.pop(t)
 		c.Assert(t, check.DeepEquals, txn)
-	case <-time.After(50 * time.Microsecond):
+	default:
 		c.Fatal("Fail to pick txn from txnManager")
 	}


select { case t := <-output: case <-time.After... }

This pattern appears a lot in this test, is it possible to extract this as a helper function to simplify the test?

lichunzhu · 2019-09-25T09:57:19Z

@suzaku PTAL

lichunzhu · 2019-09-25T10:03:37Z

/run-unit-tests

IANTHEREAL · 2019-09-25T11:04:15Z

Rest LGTM

Co-Authored-By: satoru <satorulogic@gmail.com>

lichunzhu · 2019-09-25T11:19:40Z

/run-unit-tests

july2993 · 2019-09-25T11:30:29Z

/run-all-tests tidb=release-3.0 tikv=release-3.0 pd=release-3.0

IANTHEREAL

LGTM

lichunzhu · 2019-09-25T13:27:19Z

/run-integration-tests tidb=release-3.0 tikv=release-3.0 pd=release-3.0

lichunzhu added 3 commits September 2, 2019 10:39

Change unreasonable buff size and reduce memory usage

0dfc864

Merge branch 'master' of https://github.com/pingcap/tidb-binlog into …

212d716

…czli/drainer/reduceMemoryUsage

wait for 0.01s for goroutines to update status

4520757

lichunzhu added status/WIP type/cherry-pick status/PTAL and removed status/WIP labels Sep 2, 2019

lichunzhu changed the title ~~Czli/drainer/reduce memory usage~~ drainer/: Reduce memory usage Sep 3, 2019

lichunzhu changed the title ~~drainer/: Reduce memory usage~~ drainer/: Reduce memory usage https://github.com/pingcap/tidb-binlog/pull/735 Sep 3, 2019

lichunzhu changed the title ~~drainer/: Reduce memory usage https://github.com/pingcap/tidb-binlog/pull/735~~ drainer/: Reduce memory usage Sep 3, 2019

lichunzhu changed the title ~~drainer/: Reduce memory usage~~ drainer/: Reduce memory usage (#735) Sep 3, 2019

lichunzhu requested review from WangXiangUSTC and zier-one September 4, 2019 02:50

WangXiangUSTC added status/LGT1 and removed status/PTAL labels Sep 5, 2019

suzaku reviewed Sep 5, 2019

View reviewed changes

drainer/pump_test.go Show resolved Hide resolved

suzaku previously approved these changes Sep 6, 2019

View reviewed changes

suzaku added status/LGT2 and removed status/LGT1 labels Sep 6, 2019

lichunzhu added 2 commits September 9, 2019 14:01

Merge branch 'master' of https://github.com/pingcap/tidb-binlog into …

04d71a1

…czli/drainer/reduceMemoryUsage

change defaultBinlogItemCount to 8; revise /tests/status/run.sh to sk…

8a6124a

…ip binlog by previous test case

lichunzhu dismissed suzaku’s stale review via 8a6124a September 9, 2019 06:49

fix check error

b860497

july2993 added the status/DNM label Sep 9, 2019

lichunzhu added 3 commits September 11, 2019 11:33

Merge branch 'master' of https://github.com/pingcap/tidb-binlog into …

dafce05

…czli/drainer/reduceMemoryUsage

Merge branch 'master' of https://github.com/pingcap/tidb-binlog into …

f9fc0e0

…czli/drainer/reduceMemoryUsage

Merge branch 'czli/drainer/reduceMemoryUsage' of https://github.com/l…

e21f41c

…ichunzhu/tidb-binlog into czli/drainer/reduceMemoryUsage

IANTHEREAL added status/LGT2 and removed status/LGT1 labels Sep 24, 2019

suzaku previously approved these changes Sep 25, 2019

View reviewed changes

kennytm added the status/DNM label Sep 25, 2019

add two unit tests for txnManager to test whether txnManager can quit…

03427a7

… when block at cond.Wait() or input is closed. add one unit test to test whether txnManager can handle txn whose size if bigger than maxCacheSize

lichunzhu dismissed suzaku’s stale review via 03427a7 September 25, 2019 04:44

lichunzhu added status/PTAL and removed status/DNM labels Sep 25, 2019

suzaku reviewed Sep 25, 2019

View reviewed changes

reduce wait time

21d4efc

suzaku reviewed Sep 25, 2019

View reviewed changes

simplify load_test

6789bdf

simplify failMsg

81e586c

Update pkg/loader/load_test.go

64aa654

Co-Authored-By: satoru <satorulogic@gmail.com>

IANTHEREAL approved these changes Sep 25, 2019

View reviewed changes

lichunzhu merged commit bbe5d6e into pingcap:master Sep 25, 2019

lichunzhu deleted the czli/drainer/reduceMemoryUsage branch September 25, 2019 13:52

july2993 mentioned this pull request Oct 16, 2019

TiDB Binlog high delay #768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drainer/: Reduce memory usage (#735) #737

drainer/: Reduce memory usage (#735) #737

lichunzhu commented Sep 2, 2019 •

edited

Loading

WangXiangUSTC commented Sep 5, 2019

suzaku left a comment

lichunzhu commented Sep 6, 2019

july2993 commented Sep 8, 2019 •

edited

Loading

IANTHEREAL commented Sep 24, 2019

lichunzhu commented Sep 25, 2019

IANTHEREAL commented Sep 25, 2019

lichunzhu commented Sep 25, 2019

lichunzhu commented Sep 25, 2019

suzaku Sep 25, 2019

lichunzhu Sep 25, 2019 •

edited

Loading

lichunzhu Sep 25, 2019

suzaku Sep 25, 2019

lichunzhu Sep 25, 2019

suzaku Sep 25, 2019

lichunzhu Sep 25, 2019

lichunzhu commented Sep 25, 2019

suzaku Sep 25, 2019

lichunzhu commented Sep 25, 2019

lichunzhu commented Sep 25, 2019

IANTHEREAL commented Sep 25, 2019

lichunzhu commented Sep 25, 2019

july2993 commented Sep 25, 2019

IANTHEREAL left a comment

lichunzhu commented Sep 25, 2019

drainer/: Reduce memory usage (#735) #737

drainer/: Reduce memory usage (#735) #737

Conversation

lichunzhu commented Sep 2, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

To check the effectiveness, we make two tests

TEST 1: Small binlogs, to validate the efficiency of drainer

TEST 2: the upstream data one binlog near 60M, to validate the momery usage of drainer

Check List

WangXiangUSTC commented Sep 5, 2019

suzaku left a comment

Choose a reason for hiding this comment

lichunzhu commented Sep 6, 2019

july2993 commented Sep 8, 2019 • edited Loading

IANTHEREAL commented Sep 24, 2019

lichunzhu commented Sep 25, 2019

IANTHEREAL commented Sep 25, 2019

lichunzhu commented Sep 25, 2019

lichunzhu commented Sep 25, 2019

suzaku Sep 25, 2019

Choose a reason for hiding this comment

lichunzhu Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

lichunzhu Sep 25, 2019

Choose a reason for hiding this comment

suzaku Sep 25, 2019

Choose a reason for hiding this comment

lichunzhu Sep 25, 2019

Choose a reason for hiding this comment

suzaku Sep 25, 2019

Choose a reason for hiding this comment

lichunzhu Sep 25, 2019

Choose a reason for hiding this comment

lichunzhu commented Sep 25, 2019

suzaku Sep 25, 2019

Choose a reason for hiding this comment

lichunzhu commented Sep 25, 2019

lichunzhu commented Sep 25, 2019

IANTHEREAL commented Sep 25, 2019

lichunzhu commented Sep 25, 2019

july2993 commented Sep 25, 2019

IANTHEREAL left a comment

Choose a reason for hiding this comment

lichunzhu commented Sep 25, 2019

lichunzhu commented Sep 2, 2019 •

edited

Loading

july2993 commented Sep 8, 2019 •

edited

Loading

lichunzhu Sep 25, 2019 •

edited

Loading