executor: make memory tracker for aggregate more accurate. #22463

wshwsh12 · 2021-01-21T02:47:25Z

What problem does this PR solve?

Issue Number: close #22462

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

Estimate the memory usage of aggPartialResultMapper and track it
Track memory usage of HashAggFinalWorker.
Track memory usage of GroupKey

How it Works:

Related changes

Check List

Tests

Manual test (add detailed scripts or steps below)

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM

Release note

No release note

wshwsh12 · 2021-01-24T08:43:26Z

/run-all-tests

wshwsh12 · 2021-01-28T07:50:00Z

Benchmark result: Benchmark tracks all memory allocation, but doesn't minus GC memory. So in this cases, Map memory use will be 2 times.

BenchmarkAggPartialResultMapperMemoryUsage/MapRows_0
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_0-16         	12869766	        98.7 ns/op	      80 B/op	       1 allocs/op
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_100
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_100-16       	   76569	     15660 ns/op	   12269 B/op	       9 allocs/op
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_10000
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_10000-16     	     507	   2341705 ns/op	 1611640 B/op	   10148 allocs/op
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_1000000
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_1000000-16   	       3	 346488865 ns/op	208428346 B/op	 1038292 allocs/op
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_851968
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_851968-16    	       5	 220406375 ns/op	113606825 B/op	  890112 allocs/op
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_851969
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_851969-16    	       5	 242470491 ns/op	207222587 B/op	  890200 allocs/op
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_425984
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_425984-16    	      10	 108088523 ns/op	56673632 B/op	  445000 allocs/op
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_425985
BenchmarkAggPartialResultMapperMemoryUsage/MapRows_425985-16    	       9	 119479831 ns/op	103461026 B/op	  444987 allocs/op

For example: When rownum is 1000000, B will be 18, and the estimated memory is (1<<18)*344 = 90177536B. In benchmark result, the result is 208428346B.

XuHuaiyu · 2021-01-28T06:26:51Z

executor/aggregate.go

@@ -51,15 +51,32 @@ type baseHashAggWorker struct {
 	aggFuncs     []aggfuncs.AggFunc
 	maxChunkSize int
 	stats        *AggWorkerStat
+
+	memTracker *memory.Tracker
+	BInMap     *int // incident B in Go map


This is really hard to understand for the reader who does not know the implementation of Map.
We need a more detailed comment

executor/aggregate.go

XuHuaiyu · 2021-02-03T10:10:13Z

executor/aggregate.go

+	// defBucketMemoryUsage = bucketSize*(1+unsafe.Sizeof(string) + unsafe.Sizeof(slice))+2*ptrSize
+	defBucketMemoryUsage = 8*(1+16+24) + 16
+	// Maximum average load of a bucket that triggers growth is 6.5.
+	// Represent as loadFactorNum/loadFactorDen, to allow integer math.


Why do we need allow integer math?

If we use 6.5 directly, golang complier will throw an error constant 6.5 truncated to integer in the condition len(mapper) > (1<<*w.BInMap)*loadFactorNum/loadFactorDen.
If we want it run, maybe we need add some type conversions, eg float64(len(mapper)) > float64(int(1<<*w.BInMap))*6.5. I think it is inefficient.

executor/aggregate.go

executor/executor_pkg_test.go

executor/benchmark_test.go

lzmhhh123 · 2021-02-04T08:15:44Z

executor/aggregate.go

+const (
+	// ref https://github.com/golang/go/blob/go1.15.6/src/reflect/type.go#L2162.
+	// defBucketMemoryUsage = bucketSize*(1+unsafe.Sizeof(string) + unsafe.Sizeof(slice))+2*ptrSize
+	defBucketMemoryUsage = 8*(1+16+24) + 16


Will the bucket size changed by golang in the future?

Just for curious, is there any way to set it dynamically according to different golang version?

We can check runtime.Version() to distinguish different Golang version, and using different estimation way. But we need to implement the estimation way for every different Golang version. (Of course, major version is enough).
Now Golang 1.13,1.14.1.15,1.16 uses the same map implement, so I think now we don't need to distinguish them.

executor/aggregate.go

XuHuaiyu

LGTM

lzmhhh123

LGTM

lzmhhh123 · 2021-02-18T03:11:45Z

/merge

ti-srebot · 2021-02-18T03:14:09Z

/run-all-tests

wshwsh12 added 5 commits January 20, 2021 16:05

test

3bae94a

fix

3a44c8d

Merge remote-tracking branch 'upstream/master' into test-agg-memory

bb27c78

fix

73b8f5c

fix

76fd92a

wshwsh12 requested a review from a team as a code owner January 21, 2021 02:47

wshwsh12 requested review from lzmhhh123 and removed request for a team January 21, 2021 02:47

tidy

cc553f6

github-actions bot added the sig/execution SIG execution label Jan 21, 2021

wshwsh12 removed the request for review from lzmhhh123 January 21, 2021 03:13

wshwsh12 added the status/WIP label Jan 21, 2021

wshwsh12 requested a review from XuHuaiyu January 21, 2021 06:28

wshwsh12 added 4 commits January 22, 2021 11:15

save test code

27314ee

all test info

a0b21c5

remove all test info

b972595

fmt

065e42c

wshwsh12 changed the title ~~executor: estimate the memory usage of aggPartialResultMapper~~ executor: make memory tracker for aggregate more accurate. Jan 24, 2021

Merge remote-tracking branch 'upstream/master' into test-agg-memory

5d541aa

wshwsh12 removed the status/WIP label Jan 24, 2021

XuHuaiyu added the type/enhancement The issue or PR belongs to an enhancement. label Jan 26, 2021

wshwsh12 requested a review from lzmhhh123 January 27, 2021 07:36

wshwsh12 added 3 commits January 28, 2021 14:50

add benchmark and test

0ac7c48

add benchmark and test

1280374

add version check

8d815d6

XuHuaiyu reviewed Feb 3, 2021

View reviewed changes

wshwsh12 added 2 commits February 4, 2021 11:02

address comments

3f9a42c

fix ci

59b29f4

wshwsh12 requested a review from XuHuaiyu February 4, 2021 03:25

lzmhhh123 reviewed Feb 4, 2021

View reviewed changes

address comments

92aa8ae

wshwsh12 requested a review from lzmhhh123 February 4, 2021 09:51

XuHuaiyu reviewed Feb 5, 2021

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 5, 2021

lzmhhh123 reviewed Feb 18, 2021

View reviewed changes

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 18, 2021

ti-srebot approved these changes Feb 18, 2021

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 18, 2021

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 18, 2021

Merge branch 'master' into test-agg-memory

90babc9

ti-srebot merged commit fb84db1 into pingcap:master Feb 18, 2021

mahjonp mentioned this pull request Feb 23, 2021

02-18 Performance regression on TPCH multiple queries #22881

Closed

wshwsh12 mentioned this pull request Feb 26, 2021

executor: track partialResultMap in unparalleled aggreagte. #22962

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: make memory tracker for aggregate more accurate. #22463

executor: make memory tracker for aggregate more accurate. #22463

wshwsh12 commented Jan 21, 2021 •

edited

Loading

wshwsh12 commented Jan 24, 2021

wshwsh12 commented Jan 28, 2021 •

edited

Loading

XuHuaiyu Jan 28, 2021

XuHuaiyu Feb 3, 2021

wshwsh12 Feb 4, 2021

lzmhhh123 Feb 4, 2021

wshwsh12 Feb 4, 2021

lzmhhh123 Feb 4, 2021

wshwsh12 Feb 5, 2021

XuHuaiyu left a comment

lzmhhh123 left a comment

lzmhhh123 commented Feb 18, 2021

ti-srebot commented Feb 18, 2021

executor: make memory tracker for aggregate more accurate. #22463

executor: make memory tracker for aggregate more accurate. #22463

Conversation

wshwsh12 commented Jan 21, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

wshwsh12 commented Jan 24, 2021

wshwsh12 commented Jan 28, 2021 • edited Loading

XuHuaiyu Jan 28, 2021

Choose a reason for hiding this comment

XuHuaiyu Feb 3, 2021

Choose a reason for hiding this comment

wshwsh12 Feb 4, 2021

Choose a reason for hiding this comment

lzmhhh123 Feb 4, 2021

Choose a reason for hiding this comment

wshwsh12 Feb 4, 2021

Choose a reason for hiding this comment

lzmhhh123 Feb 4, 2021

Choose a reason for hiding this comment

wshwsh12 Feb 5, 2021

Choose a reason for hiding this comment

XuHuaiyu left a comment

Choose a reason for hiding this comment

lzmhhh123 left a comment

Choose a reason for hiding this comment

lzmhhh123 commented Feb 18, 2021

ti-srebot commented Feb 18, 2021

wshwsh12 commented Jan 21, 2021 •

edited

Loading

wshwsh12 commented Jan 28, 2021 •

edited

Loading