executor: make memory tracker for aggregate more accurate.#22463
executor: make memory tracker for aggregate more accurate.#22463ti-srebot merged 18 commits intopingcap:masterfrom
Conversation
|
/run-all-tests |
|
Benchmark result: Benchmark tracks all memory allocation, but doesn't minus GC memory. So in this cases, Map memory use will be 2 times. 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. |
executor/aggregate.go
Outdated
| stats *AggWorkerStat | ||
|
|
||
| memTracker *memory.Tracker | ||
| BInMap *int // incident B in Go map |
There was a problem hiding this comment.
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
Outdated
| // 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. |
There was a problem hiding this comment.
Why do we need allow integer math?
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
Will the bucket size changed by golang in the future?
There was a problem hiding this comment.
Just for curious, is there any way to set it dynamically according to different golang version?
There was a problem hiding this comment.
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.
|
/merge |
|
/run-all-tests |
What problem does this PR solve?
Issue Number: close #22462
Problem Summary:
What is changed and how it works?
Proposal: xxx
What's Changed:
How it Works:
Related changes
Check List
Tests
Side effects
Release note