You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/04/09 05:31:38 UTC
[GitHub] [incubator-doris] vagetablechicken opened a new pull request #3286:
unregister fragment mem tracker in close()
vagetablechicken opened a new pull request #3286: unregister fragment mem tracker in close()
URL: https://github.com/apache/incubator-doris/pull/3286
ref https://github.com/apache/incubator-doris/issues/3273
P.S.
https://github.com/apache/incubator-doris/blob/614a76beeac73821c78903c46e7a703b7956796b/be/src/runtime/plan_fragment_executor.cpp#L559-L562
I think this piece of code is useless.
This `_mem_tracker` in `PlanFragmentExecutor` is set as fragment_mem_tracker of `RuntimeState`.
**direct use**
We use it in these code, when rowbatch reset, mem tracker's consumption will be released.
https://github.com/apache/incubator-doris/blob/7eab12a40e5a20959c13eba99697c171d1461e0b/be/src/exec/olap_rewrite_node.cpp#L57-L58
https://github.com/apache/incubator-doris/blob/839ec45197fb4559c4946bf0fa4ce3b6805b4248/be/src/exec/olap_scan_node.cpp#L1217-L1218
**other usage**
e.g.
https://github.com/apache/incubator-doris/blob/6c33f805449d9a7bc8809a575407acfa87bb061e/be/src/exec/olap_scanner.cpp#L245
won't consume the fragment mem tracker. We don't need to worry about the fragment mem tracker consumption is not zero when we want to destroy it.
Or we can add a consumption check before we close the mem tracker?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay merged pull request #3286: unregister
fragment mem tracker in close()
Posted by GitBox <gi...@apache.org>.
imay merged pull request #3286: unregister fragment mem tracker in close()
URL: https://github.com/apache/incubator-doris/pull/3286
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #3286:
unregister fragment mem tracker in close()
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #3286: unregister fragment mem tracker in close()
URL: https://github.com/apache/incubator-doris/pull/3286#discussion_r405984076
##########
File path: be/src/runtime/plan_fragment_executor.cpp
##########
@@ -556,10 +556,12 @@ void PlanFragmentExecutor::close() {
}
}
- // _mem_tracker init failed
+ // fragment mem tracker needs unregister
if (_mem_tracker.get() != nullptr) {
- _mem_tracker->release(_mem_tracker->consumption());
+ _mem_tracker->unregister_from_parent();
+ _mem_tracker->close();
Review comment:
Why not keep origin code and just add `unregister_from_parent`.
If you reset this mem_tracker here, I'm not sure if it will work in some situation, some runtime holder may access this tracker.
So, just to keep things simple, better not change origin code.
By the way, this tracker can be substituted by other tracker. Maybe it can be done in another issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] vagetablechicken commented on a change in pull
request #3286: unregister fragment mem tracker in close()
Posted by GitBox <gi...@apache.org>.
vagetablechicken commented on a change in pull request #3286: unregister fragment mem tracker in close()
URL: https://github.com/apache/incubator-doris/pull/3286#discussion_r406581630
##########
File path: be/src/runtime/plan_fragment_executor.cpp
##########
@@ -556,10 +556,12 @@ void PlanFragmentExecutor::close() {
}
}
- // _mem_tracker init failed
+ // fragment mem tracker needs unregister
if (_mem_tracker.get() != nullptr) {
- _mem_tracker->release(_mem_tracker->consumption());
+ _mem_tracker->unregister_from_parent();
+ _mem_tracker->close();
Review comment:
I searched all usages of `_runtime_state->fragment_mem_tracker()`. Where else?
How about move this piece to PlanFragmentExecutor dtor? I think it's a better place.
> By the way, this tracker can be substituted by other tracker. Maybe it can be done in another issue.
Can't agree more. fragment mem tracker's label is "fragment mem-limit", it's not very useful. And one plan has two level1 nodes, I think it's not a good design.
Let's create a new issue to discuss it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org