import: fix memory leak #39332

dsdashun · 2022-11-23T04:45:31Z

What problem does this PR solve?

Issue Number: close #39331

Problem Summary:

What is changed and how it works?

When kvMemBuf reuses the byte buffer in the available byte buffer pool, iterate on all the byte buffer's capacity. If it can find at least one byte buffer with a large capacity, it will choose to reuse it, rather than create a new byte buffer.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
compile the modified version, and check whether memory leak has been fixed. Here are the heap profiles of the Lightning process after the modification:
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-11-23T04:45:33Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
lance6716

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.

dsdashun · 2022-11-23T04:46:09Z

/run-integration-br-test

dsdashun · 2022-11-23T06:29:59Z

integration br test passed: https://ci.pingcap.net/blue/organizations/jenkins/tidb_ghpr_integration_br_test/detail/tidb_ghpr_integration_br_test/11150/pipeline

dsdashun · 2022-11-23T06:30:56Z

/cc @3pointer @lance6716

lance6716

seems we can add a simple unit test that after some AllocateBuf/Set and Recycle, the mb.availableBufs does not grow badly

D3Hunter · 2022-11-25T03:44:18Z

br/pkg/lightning/backend/kv/session.go

+		existingBuf    *bytesBuf
+		existingBufIdx int
+	)
+	for i, buf := range mb.availableBufs {


should we limit max length of mb.availableBufs? in case encodeLoop is slow down for we might loop through all availableBufs on every AllocateBuf in worst case

Maybe no need to limit the max length. There are two reasons:

the allocation size is by the power of 2, starting from 1MB. So the possible byte buffer capacities are 1MB, 2MB, 4MB, ... Since the memory limit is typically around 256GB, and the reuse mechanism has fixed now, in the extreme case without any reuse, the available byte buffer pool's size is around 18 (1MB, 2MB, 4MB, .... 256GB), otherwise a reuse will happen .

the typical allocation size will fall into only some byte buffer sizes. Since the reuse mechanism has fixed now, the reuse will happen frequently. So the overall availableBufs size is controllable.

But it is OK to add a defending mechanism for the performance degration here.

Fixed in a71021c

dsdashun · 2022-11-26T08:24:32Z

seems we can add a simple unit test that after some AllocateBuf/Set and Recycle, the mb.availableBufs does not grow badly

Fixed in a71021c

dsdashun · 2022-11-26T08:26:21Z

/run-integration-br-test

dsdashun · 2022-11-27T03:17:55Z

/run-integration-br-test

dsdashun · 2022-11-27T06:28:23Z

integration br test passed: https://ci.pingcap.net/blue/organizations/jenkins/tidb_ghpr_integration_br_test/detail/tidb_ghpr_integration_br_test/11182/pipeline

lance6716 · 2022-11-28T02:19:20Z

br/pkg/lightning/backend/kv/session_internal_test.go

+}
+
+func TestKVMemBufBatchAllocAndRecycle(t *testing.T) {
+	type testCase struct {


this type is not used?

dsdashun · 2022-11-28T03:06:16Z

/merge

ti-chi-bot · 2022-11-28T03:06:20Z

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

Commit hash: 0174d56

ti-chi-bot · 2022-11-28T03:34:43Z

In response to a cherrypick label: new pull request created: #39407.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2022-11-28T03:35:25Z

In response to a cherrypick label: new pull request created: #39408.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2022-11-28T03:36:06Z

In response to a cherrypick label: new pull request created: #39409.

Signed-off-by: ti-chi-bot <[email protected]>

sre-bot · 2022-11-28T04:08:16Z

TiDB MergeCI notify

✅ Well Done! New fixed [1] after this pr merged.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-ddl-test	✅ all 6 tests passed	14 min	Fixed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 17 tests passed	28 min	Existing passed
idc-jenkins-ci/integration-cdc-test	🟢 all 39 tests passed	22 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	11 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	8 min 12 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	6 min 29 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	6 min 14 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 34 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 5 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

close #39331

This reverts commit 4b777b4.

close #39331

import: fix memory leak

b8426bc

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 23, 2022

ti-chi-bot requested review from 3pointer and lance6716 November 23, 2022 06:30

lance6716 reviewed Nov 23, 2022

View reviewed changes

D3Hunter reviewed Nov 25, 2022

View reviewed changes

dsdashun added 2 commits November 26, 2022 16:22

address comments

a71021c

Merge remote-tracking branch 'origin/master' into fix_mem_leak

7eb6b5d

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 26, 2022

add missing bazel file

0174d56

D3Hunter approved these changes Nov 28, 2022

View reviewed changes

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

lance6716 approved these changes Nov 28, 2022

View reviewed changes

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

Merge branch 'master' into fix-39331

c246754

ti-chi-bot merged commit 5c9570d into pingcap:master Nov 28, 2022

ti-chi-bot mentioned this pull request Nov 28, 2022

import: fix memory leak (#39332) #39407

Closed

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Nov 28, 2022

This is an automated cherry-pick of pingcap#39332

27a9f91

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Nov 28, 2022

import: fix memory leak (#39332) #39408

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Nov 28, 2022

This is an automated cherry-pick of pingcap#39332

6ce4bff

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Nov 28, 2022

import: fix memory leak (#39332) #39409

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Nov 28, 2022

This is an automated cherry-pick of pingcap#39332

9233231

Signed-off-by: ti-chi-bot <[email protected]>

dsdashun deleted the fix-39331 branch November 28, 2022 08:39

ti-chi-bot added a commit that referenced this pull request Dec 5, 2022

import: fix memory leak (#39332) (#39408)

4b777b4

close #39331

MimeLyc added a commit that referenced this pull request Dec 5, 2022

Revert "import: fix memory leak (#39332) (#39408)"

a771b9c

This reverts commit 4b777b4.

ti-chi-bot added a commit that referenced this pull request Jan 18, 2023

import: fix memory leak (#39332) (#39409)

30c22da

close #39331

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import: fix memory leak #39332

import: fix memory leak #39332

dsdashun commented Nov 23, 2022

ti-chi-bot commented Nov 23, 2022 •

edited

Loading

dsdashun commented Nov 23, 2022

dsdashun commented Nov 23, 2022

dsdashun commented Nov 23, 2022

lance6716 left a comment

D3Hunter Nov 25, 2022

dsdashun Nov 25, 2022

dsdashun Nov 26, 2022

dsdashun commented Nov 26, 2022

dsdashun commented Nov 26, 2022

dsdashun commented Nov 27, 2022

dsdashun commented Nov 27, 2022

lance6716 Nov 28, 2022

dsdashun commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022

sre-bot commented Nov 28, 2022

import: fix memory leak #39332

import: fix memory leak #39332

Conversation

dsdashun commented Nov 23, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 23, 2022 • edited Loading

dsdashun commented Nov 23, 2022

dsdashun commented Nov 23, 2022

dsdashun commented Nov 23, 2022

lance6716 left a comment

Choose a reason for hiding this comment

D3Hunter Nov 25, 2022

Choose a reason for hiding this comment

dsdashun Nov 25, 2022

Choose a reason for hiding this comment

dsdashun Nov 26, 2022

Choose a reason for hiding this comment

dsdashun commented Nov 26, 2022

dsdashun commented Nov 26, 2022

dsdashun commented Nov 27, 2022

dsdashun commented Nov 27, 2022

lance6716 Nov 28, 2022

Choose a reason for hiding this comment

dsdashun commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022

ti-chi-bot commented Nov 28, 2022

sre-bot commented Nov 28, 2022

TiDB MergeCI notify

ti-chi-bot commented Nov 23, 2022 •

edited

Loading