executor: more precise memory tracking #35785

mengxin9014 · 2022-06-28T07:37:50Z

What problem does this PR solve?

Issue Number: close #36018

Problem Summary:
Memory tracking is not accurate enough.

What is changed and how it works?

Pre-allocate the size of slice to prevent adding too much memory when appending.

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.

more precise memory tracking in hash aggregation.

ti-chi-bot · 2022-06-28T07:37:51Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

XuHuaiyu
wshwsh12

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.

sre-bot · 2022-06-28T07:51:21Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/2e1663b41160f9b535479b562882c7ebc5a6a39f

wshwsh12 · 2022-07-14T06:00:44Z

executor/aggregate.go

+	w.groupKeys = make([][]byte, len(w.groupSet.StringSet))
+	groupKeysIndex := 0
 	for groupKey := range w.groupSet.StringSet {
-		w.groupKeys = append(w.groupKeys, []byte(groupKey))
+		w.groupKeys[groupKeysIndex] = []byte(groupKey)
+		groupKeysIndex++


w.groupKeys = make([][]byte, 0, len(w.groupSet.StringSet)) for xxxx { w.groupKeys = append(w.groupKeys, []byte(groupKey)) }

…gxin9014/tidb into mx/preassignForPreciseMemoryTracking

wshwsh12 · 2022-08-01T07:44:45Z

executor/aggregate.go

@@ -671,7 +673,7 @@ func (w *HashAggFinalWorker) consumeIntermData(sctx sessionctx.Context) (err err
 			memSize := getGroupKeyMemUsage(w.groupKeys)
 			w.groupKeys = w.groupKeys[:0]
 			for i := 0; i < groupKeysLen; i++ {
-				w.groupKeys = append(w.groupKeys, []byte(groupKeys[i]))
+				w.groupKeys = append(w.groupKeys, hack.Slice(groupKeys[i]))
 			}
 			failpoint.Inject("ConsumeRandomPanic", nil)
 			w.memTracker.Consume(getGroupKeyMemUsage(w.groupKeys) - memSize)


Only need consider the slice size here. We don't need to consume the memory usage of string's data, because L676 is shallow copy.

XuHuaiyu · 2022-08-10T07:35:20Z

executor/aggregate.go

@@ -631,6 +634,13 @@ func (w *baseHashAggWorker) getPartialResult(sc *stmtctx.StatementContext, group
 	return partialResults
 }

+func (w *baseHashAggWorker) getPartialResultSize() int {


getPartialResultSliceLenConsiderByteAlign ?

getPartialResultSliceLenConsiderByteAlign ?

done.

XuHuaiyu · 2022-08-10T07:35:56Z

executor/aggregate.go

@@ -613,11 +613,14 @@ func (w *baseHashAggWorker) getPartialResult(sc *stmtctx.StatementContext, group
 		if partialResults[i], ok = mapper[string(groupKey[i])]; ok {
 			continue
 		}
-		for _, af := range w.aggFuncs {
+		partialResultSize := w.getPartialResultSize()


move this line out of the loop

move this line out of the loop

done.

mengxin9014 · 2022-08-10T08:35:58Z

/run-all-tests

mengxin9014 · 2022-08-10T11:17:03Z

/merge

ti-chi-bot · 2022-08-10T11:17:05Z

@mengxin9014: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

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.

wshwsh12 · 2022-08-10T14:19:51Z

/merge

ti-chi-bot · 2022-08-10T14:19:54Z

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

Commit hash: d8a8711

sre-bot · 2022-08-10T15:21:15Z

TiDB MergeCI notify

🔴 Bad News! [4] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/common-test	🔴 failed 2, success 9, total 11	13 min	Existing failure
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 3, success 14, total 17	10 min	Existing failure
idc-jenkins-ci-tidb/sqllogic-test-2	🔴 failed 2, success 26, total 28	5 min 1 sec	Existing failure
idc-jenkins-ci-tidb/sqllogic-test-1	🔴 failed 3, success 23, total 26	3 min 53 sec	Existing failure
idc-jenkins-ci/integration-cdc-test	🟢 all 36 tests passed	31 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	8 min 12 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	5 min 32 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 22 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 32 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

…verage * upstream/master: (98 commits) planner: provide a unified interface to set and get user variables and types (pingcap#37046) *: fix check-2 to bazel (pingcap#37055) distsql: revert 37019 (pingcap#37048) planner: introduce new functions for the general statement in session interface (pingcap#37024) session: make BenchmarkCompileExecutePreparedStmt update to date (pingcap#36960) server: Change proxyprotocol to lazy mode (pingcap#36301) test: fix `TestDDLSchedulingMultiTimes` and `TestConcurrentDDLSwitch` timeout (pingcap#37043) makefile: add bazel realtikvtest (pingcap#37042) distsql: make analyze request SI (pingcap#37019) *: gofmt code for adapting go 1.19 (pingcap#36832) planner: fix data race in the TestInlineProjection4HashJoinIssue15316 (pingcap#37014) planner: introduce Session.CacheGeneralStmt for general plan cache (pingcap#37028) server: revise scheme code (pingcap#37021) planner: fix unstable test TestPushDownSelectionForMPP (pingcap#37038) executor: more precise memory tracking (pingcap#35785) executor: add privilege check for prepare stmt (pingcap#36933) *: optimize temptable.SessionSnapshotInterceptor (pingcap#36999) statistics: batch insert topn and buckets to storage to speed up loading stats (pingcap#36948) planner: remove keyword `prepared` in plan cache backend (pingcap#37023) util: fix unstable test TestCheckCN (pingcap#37016) ...

precise memory tracking

e58abd3

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

dddba4e

mengxin9014 changed the title ~~WIP: executor: precise memory tracking~~ executor: precise memory tracking Jul 7, 2022

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 Jul 7, 2022

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

286535a

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/needs-linked-issue labels Jul 7, 2022

mengxin9014 changed the title ~~executor: precise memory tracking~~ executor: more precise memory tracking in hash aggregation Jul 7, 2022

ti-chi-bot removed the release-note-none Denotes a PR that doesn't merit a release note. label Jul 7, 2022

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

fffaa61

wshwsh12 self-requested a review July 12, 2022 07:37

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

cc00efa

wshwsh12 reviewed Jul 14, 2022

View reviewed changes

mengxin9014 added 7 commits July 15, 2022 14:27

modify according to comments

2ebf3cd

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

8cf2d53

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

cfc2fd1

fix some problems

7e71a9a

Merge branch 'mx/preassignForPreciseMemoryTracking' of github.com:men…

3e1139d

…gxin9014/tidb into mx/preassignForPreciseMemoryTracking

modify according to comments

5390571

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

38851b9

wshwsh12 reviewed Aug 1, 2022

View reviewed changes

mengxin9014 added 2 commits August 9, 2022 16:00

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

619ef73

memory tracking is more accurate

7682a0f

ti-chi-bot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 9, 2022

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

refactor

cd21e7a

mengxin9014 force-pushed the mx/preassignForPreciseMemoryTracking branch from a474b86 to cd21e7a Compare August 10, 2022 06:38

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 10, 2022

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

bcc801c

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2022

delete unuseful code

569e4c6

mengxin9014 requested a review from wshwsh12 August 10, 2022 06:47

modify some problem

6001cba

XuHuaiyu reviewed Aug 10, 2022

View reviewed changes

mengxin9014 added 2 commits August 10, 2022 15:37

rename function

bf8e56c

move init out of loop

6eb1758

mengxin9014 force-pushed the mx/preassignForPreciseMemoryTracking branch from 10621a2 to 6eb1758 Compare August 10, 2022 07:45

wshwsh12 approved these changes Aug 10, 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 Aug 10, 2022

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

d8a8711

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

Merge branch 'master' into mx/preassignForPreciseMemoryTracking

2e1663b

ti-chi-bot merged commit 8af73ff into pingcap:master Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: more precise memory tracking #35785

executor: more precise memory tracking #35785

mengxin9014 commented Jun 28, 2022 •

edited

Loading

ti-chi-bot commented Jun 28, 2022 •

edited

Loading

sre-bot commented Jun 28, 2022 •

edited

Loading

wshwsh12 Jul 14, 2022

wshwsh12 Aug 1, 2022

XuHuaiyu Aug 10, 2022

mengxin9014 Aug 10, 2022

XuHuaiyu Aug 10, 2022

mengxin9014 Aug 10, 2022

mengxin9014 commented Aug 10, 2022

mengxin9014 commented Aug 10, 2022

ti-chi-bot commented Aug 10, 2022

wshwsh12 commented Aug 10, 2022

ti-chi-bot commented Aug 10, 2022

sre-bot commented Aug 10, 2022

executor: more precise memory tracking #35785

executor: more precise memory tracking #35785

Conversation

mengxin9014 commented Jun 28, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jun 28, 2022 • edited Loading

sre-bot commented Jun 28, 2022 • edited Loading

wshwsh12 Jul 14, 2022

Choose a reason for hiding this comment

wshwsh12 Aug 1, 2022

Choose a reason for hiding this comment

XuHuaiyu Aug 10, 2022

Choose a reason for hiding this comment

mengxin9014 Aug 10, 2022

Choose a reason for hiding this comment

XuHuaiyu Aug 10, 2022

Choose a reason for hiding this comment

mengxin9014 Aug 10, 2022

Choose a reason for hiding this comment

mengxin9014 commented Aug 10, 2022

mengxin9014 commented Aug 10, 2022

ti-chi-bot commented Aug 10, 2022

wshwsh12 commented Aug 10, 2022

ti-chi-bot commented Aug 10, 2022

sre-bot commented Aug 10, 2022

TiDB MergeCI notify

mengxin9014 commented Jun 28, 2022 •

edited

Loading

ti-chi-bot commented Jun 28, 2022 •

edited

Loading

sre-bot commented Jun 28, 2022 •

edited

Loading