Skip to content

Conversation

@vagetablechicken
Copy link
Contributor

@vagetablechicken vagetablechicken commented Jul 21, 2020

Ref #3714
We make all MemTrackers shared, in order to show MemTracker real-time consumptions on the web.
As follows:

  1. nearly all MemTracker raw ptr -> shared_ptr
  2. Use CreateTracker() to create new MemTracker(in order to add itself to its parent)
  3. RowBatch & MemPool still use raw ptrs of MemTracker, it's esay to ensure RowBatch & MemPool dtor exec before MemTracker's dtor. So we don't change these code.
  4. MemTracker can use RumtimeProfile's counter to calc consumption. So RuntimeProfile's counter need to be shared too. We add a shared counter pool to store the shared counter, don't change other counters of RuntimeProfile.

Note that, this PR doesn't change the MemTracker tree structure. So there still have some orphan trackers, e.g. RowBlockV2's MemTracker. If you find some shared MemTrackers are little mem consumption & too time-consuming, you could make them be the orphan, then it's fine to use the raw ptr.

@HappenLee
Copy link
Contributor

Hello, I have two questions to ask。

  1. shared ptr use for avoid memory leaks. but at the same time, it brings about several problems:
    (1)There are additional costs of ref count of ptr
    (2) The interface becomes infectious
    So Is it good enough to do this replacement of shared_ptr ? what benefit us?

  2. unique_ptr runtime expenses is close to the raw pointer and it's simple enough to use。
    In most cases, should we replacing it with shared_prt.

@vagetablechicken
Copy link
Contributor Author

vagetablechicken commented Jul 22, 2020

Hello, I have two questions to ask。

  1. shared ptr use for avoid memory leaks. but at the same time, it brings about several problems:
    (1)There are additional costs of ref count of ptr
    (2) The interface becomes infectious
    So Is it good enough to do this replacement of shared_ptr ? what benefit us?
  2. unique_ptr runtime expenses is close to the raw pointer and it's simple enough to use。
    In most cases, should we replacing it with shared_prt.

As metioned in the first comment, this pr is for showing all MemTracker on BE's website #3714 , like kudu. So we need to make all MemTrackers not owned by anyone. If we get it on web, it shouldn't be destoryed with the obj which created it.

@kangkaisen
Copy link
Contributor

Hello, I have two questions to ask。

  1. shared ptr use for avoid memory leaks. but at the same time, it brings about several problems:
    (1)There are additional costs of ref count of ptr
    (2) The interface becomes infectious
    So Is it good enough to do this replacement of shared_ptr ? what benefit us?
  2. unique_ptr runtime expenses is close to the raw pointer and it's simple enough to use。
    In most cases, should we replacing it with shared_prt.

As metioned in the first comment, this pr is for showing all MemTracker on BE's website #3714 , like kudu. So we need to make all MemTrackers not owned by anyone. If we get it on web, it shouldn't be destoryed with the obj which created it.

So the mainly reason use shared_prt is only to show MemTracker on website?

@vagetablechicken
Copy link
Contributor Author

vagetablechicken commented Jul 22, 2020

So the mainly reason use shared_prt is only to show MemTracker on website?

Yes. We had met a lot of problems of memory, they usually are accompanied with large MemPool consumption. But we can't figure out, MemPool consumption is the total metric. I think showing MemTracker is valuable.

@HappenLee
Copy link
Contributor

Hello, I have two questions to ask。

  1. shared ptr use for avoid memory leaks. but at the same time, it brings about several problems:
    (1)There are additional costs of ref count of ptr
    (2) The interface becomes infectious
    So Is it good enough to do this replacement of shared_ptr ? what benefit us?
  2. unique_ptr runtime expenses is close to the raw pointer and it's simple enough to use。
    In most cases, should we replacing it with shared_prt.

As metioned in the first comment, this pr is for showing all MemTracker on BE's website #3714 , like kudu. So we need to make all MemTrackers not owned by anyone. If we get it on web, it shouldn't be destoryed with the obj which created it.

The lifecycle of MemTracker for a query is clear。Now RunningProfile don't use shared ptr, also can show running statistics of query. So it 's really necessary to shared_ptr to manage lifecycles?Can it be a tree structure like the runningprofile?

@vagetablechicken
Copy link
Contributor Author

The lifecycle of MemTracker for a query is clear。Now RunningProfile don't use shared ptr, also can show running statistics of query. So it 's really necessary to shared_ptr to manage lifecycles?Can it be a tree structure like the runningprofile?

It's not for the lifecycle of MemTracker. How do you know the exact mem consumption of one query? What if the MemTracker node destoryed, when we are traversing the tree?

chaoyli
chaoyli previously approved these changes Jul 29, 2020
@vagetablechicken vagetablechicken requested a review from chaoyli July 31, 2020 06:46
@chaoyli chaoyli merged commit 10f822e into apache:master Jul 31, 2020
@morningman morningman mentioned this pull request Aug 4, 2020
2 tasks
morningman pushed a commit that referenced this pull request Aug 4, 2020
after making MemTracker shared(#4135), some code haven't been fixed,
and add some useless ut back to build. Fixed in this pr.
morningman added a commit that referenced this pull request Aug 18, 2020
…g usage (#4345)

After PR: #4135, If a mem tracker has parent, it should be created by 'CreateTracker'.
So I removed other unused constructors.

And also fix the bug described in #4344
acelyc111 pushed a commit to acelyc111/incubator-doris that referenced this pull request Aug 21, 2020
…g usage (apache#4345)

After PR: apache#4135, If a mem tracker has parent, it should be created by 'CreateTracker'.
So I removed other unused constructors.

And also fix the bug described in apache#4344
wangbo pushed a commit to wangbo/incubator-doris that referenced this pull request Sep 2, 2020
…g usage (apache#4345)

After PR: apache#4135, If a mem tracker has parent, it should be created by 'CreateTracker'.
So I removed other unused constructors.

And also fix the bug described in apache#4344
wangbo pushed a commit to wangbo/incubator-doris that referenced this pull request Sep 2, 2020
…g usage (apache#4345)

After PR: apache#4135, If a mem tracker has parent, it should be created by 'CreateTracker'.
So I removed other unused constructors.

And also fix the bug described in apache#4344
acelyc111 pushed a commit to acelyc111/incubator-doris that referenced this pull request Jan 20, 2021
We make all MemTrackers shared, in order to show MemTracker real-time consumptions on the web.
As follows:
1. nearly all MemTracker raw ptr -> shared_ptr
2. Use CreateTracker() to create new MemTracker(in order to add itself to its parent)
3. RowBatch & MemPool still use raw ptrs of MemTracker, it's easy to ensure RowBatch & MemPool destructor exec
     before MemTracker's destructor. So we don't change these code.
4. MemTracker can use RuntimeProfile's counter to calc consumption. So RuntimeProfile's counter need to be shared
    too. We add a shared counter pool to store the shared counter, don't change other counters of RuntimeProfile.
Note that, this PR doesn't change the MemTracker tree structure. So there still have some orphan trackers, e.g. RowBlockV2's MemTracker. If you find some shared MemTrackers are little memory consumption & too time-consuming, you could make them be the orphan, then it's fine to use the raw ptr.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants