-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature-wip] (memory tracker) (step4) Switch TLS mem tracker to separate more detailed memory usage #8669
Conversation
51be474
to
66350b6
Compare
66350b6
to
66ee4fa
Compare
82a3cde
to
d16b88c
Compare
d16b88c
to
ee3e06b
Compare
The purpose of this PR is to add all switch thread mem trackers in all appropriate places and ensure overall performance and accuracy. At present, the statistics of Load task and other details are inaccurate, and will be fixed in the follow-up. In addition, there is a problem with Local Test that has not been completely resolved, in commit |
@@ -217,14 +217,14 @@ inline void ThreadMemTrackerMgr::cache_consume(int64_t size) { | |||
if (_untracked_mem >= config::mem_tracker_consume_min_size_bytes || | |||
_untracked_mem <= -config::mem_tracker_consume_min_size_bytes) { | |||
DCHECK(_untracked_mems.find(_tracker_id) != _untracked_mems.end()); | |||
start_thread_mem_tracker = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment to explain why adding start_thread_mem_tracker = false;
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid getting into infinite recursion if there is a temporary memory allocation in mem_tracker.consume/try_consume.
It is mentioned in the comments of tcmalloc_hook.h
: Allocating memory in the Hook command causes the Hook to be entered again, infinite recursion.
This needs to ensure that all memory allocated in mem_tracker.consume/try_consume is freed in time to avoid tracking misses.
Add comments when modifying uniformly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
…rate more detailed memory usage (apache#8669) Based on apache#8605, Separate out the memory usage of each operator from the Query/Load/StorageEngine mem tracker.
…rate more detailed memory usage (apache#8669) Based on apache#8605, Separate out the memory usage of each operator from the Query/Load/StorageEngine mem tracker.
…rate more detailed memory usage (apache#8669) Based on apache#8605, Separate out the memory usage of each operator from the Query/Load/StorageEngine mem tracker.
Proposed changes
Issue Number: close #7196
Problem Summary:
Based on #8605, Separate out the memory usage of each operator from the Query/Load/StorageEngine mem tracker.
Checklist(Required)
Further comments
1. Accuracy verification
Test SQL
Test Load
Stream load ssb::LINEORDER, 600w
TODO: At present, the value of load task tracker is negative, and some malloc memory may not be recorded, or the malloc memory in other places may be freed.
2. Pformance Testing
As of now, the new memory statistics framework will bring about a 1%-2% performance penalty.
Env: 1 FE, 1 BE
Test Set: ssb LINEORDER 600w
set parallel_fragment_exec_instance_num = 10;
set exec_mem_limit = 21474836480;
TEST 1 - Big query
Test SQL:
Result:
TEST 2 - Small query
Test SQL:
set untracked_mem_limit_mbytes=2M;
Result:
3. Future
memory_leak_detection
is still experimental;