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 2022/05/03 09:34:46 UTC
[GitHub] [incubator-doris] dataroaring opened a new pull request, #9350: remove explicit memtracker from scannode
dataroaring opened a new pull request, #9350:
URL: https://github.com/apache/incubator-doris/pull/9350
memory is tracked by implicit tracker via hook.
# Proposed changes
Issue Number: close #xxx
## Problem Summary:
Describe the overview of changes.
## Checklist(Required)
1. Does it affect the original behavior: (Yes/No/I Don't know)
2. Has unit tests been added: (Yes/No/No Need)
3. Has document been added or modified: (Yes/No/No Need)
4. Does it need to update dependencies: (Yes/No)
5. Are there any changes that cannot be rolled back: (Yes/No)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xinyiZzz commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
xinyiZzz commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1116889597
What is the reason for removing `_buffered_bytes` and `_block_mem_tracker`?
`_block_mem_tracker` also exists elsewhere, reusing the previous memory track logic,
If we don't need to track more fine-grained block memory, I would consider removing both. @dataroaring @morningman
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xinyiZzz commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
xinyiZzz commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1118268486
> > > > > _block_mem_tracker
> > > >
> > > >
> > > > So why remove `counterpart`, `counterpart` is just for `_block_mem_tracker`.
> > >
> > >
> > > It seems that _block_mem_tracker tracks memory for vectorized block? If so, we should track them via ExecNode::_mem_tracker， and it should be tracked via hook, right? btw, we should track mmap allocated memory.
> >
> >
> > mmap has been tracked and introduced in pr: #9145
> > `_block_mem_tracker` only tracks memory for vectorized block, `ExecNode::_mem_tracker` includes other memory in ExecNode in addition to memory for vectorized block. This is the purpose of my previous design, so my reply above is whether it is necessary.
>
> Yep. I understand what you mean. IMHO, It should be case by case, e.g. for the OlapScanNode, the majority memory should be contributed by blocks, so value of block_mem_tracker is about equal value of ExecNode::_mem_tracker？ But for the AggregateNode, may be the majority memory should be contributed by hash table, so we should track memory usage of hash table.
I agree with you that the `_block_mem_tracker` of `scan node` is really unnecessary. After the new MemTracker is used for a period of time, the more detailed tracker and the existing virtual tracker can be modified.
A little leak in the code:
volap_scan_node.h:67 - remove `_block_mem_tracker` definition.
volap_scan_node.cpp:323 - remove `_block_mem_tracker` create.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] dataroaring commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
dataroaring commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1120405697
> > > > > > _block_mem_tracker
> > > > >
> > > > >
> > > > > So why remove `counterpart`, `counterpart` is just for `_block_mem_tracker`.
> > > >
> > > >
> > > > It seems that _block_mem_tracker tracks memory for vectorized block? If so, we should track them via ExecNode::_mem_tracker， and it should be tracked via hook, right? btw, we should track mmap allocated memory.
> > >
> > >
> > > mmap has been tracked and introduced in pr: #9145
> > > `_block_mem_tracker` only tracks memory for vectorized block, `ExecNode::_mem_tracker` includes other memory in ExecNode in addition to memory for vectorized block. This is the purpose of my previous design, so my reply above is whether it is necessary.
> >
> >
> > Yep. I understand what you mean. IMHO, It should be case by case, e.g. for the OlapScanNode, the majority memory should be contributed by blocks, so value of block_mem_tracker is about equal value of ExecNode::_mem_tracker？ But for the AggregateNode, may be the majority memory should be contributed by hash table, so we should track memory usage of hash table.
>
> I agree with you that the `_block_mem_tracker` of `scan node` is really unnecessary. After the new MemTracker is used for a period of time, the more detailed tracker and the existing virtual tracker are expected to be modified.
>
> A little leak in the code: volap_scan_node.h:67 - remove `_block_mem_tracker` definition. volap_scan_node.cpp:323 - remove `_block_mem_tracker` create.
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #9350:
URL: https://github.com/apache/doris/pull/9350#issuecomment-1304350905
We're closing this PR because it hasn't been updated in a while.
This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and feel free a maintainer to remove the Stale tag!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] dataroaring commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
dataroaring commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1117263656
> `_block_mem_tracker` also exists in other places. I intentionally kept the previous memory track logic and tracked the more detailed block memory. If we don't need track block memory, or it will bring performance or other issues, then I will consider removing both. @dataroaring @morningman
I removed it because its counterpart in olap_scan_node.cpp is removed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xinyiZzz commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
xinyiZzz commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1120410466
LGTM
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xinyiZzz commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
xinyiZzz commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1118087890
> _block_mem_tracker
So why remove `counterpart`, `counterpart` is just for `_block_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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] github-actions[bot] commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1116220705
PR approved by at least one committer and no changes requested.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] github-actions[bot] commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1116220748
PR approved by anyone and no changes requested.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xinyiZzz commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
xinyiZzz commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1118239190
> > > _block_mem_tracker
> >
> >
> > So why remove `counterpart`, `counterpart` is just for `_block_mem_tracker`.
>
> It seems that _block_mem_tracker tracks memory for vectorized block? If so, we should track them via ExecNode::_mem_tracker， and it should be tracked via hook, right? btw, we should track mmap allocated memory.
mmap has been tracked and introduced in pr: https://github.com/apache/incubator-doris/pull/9145
`_block_mem_tracker` only tracks memory for vectorized block, `ExecNode::_mem_tracker` includes other memory in ExecNode in addition to memory for vectorized block. This is the purpose of my previous design, so my reply above is whether it is necessary.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] closed pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #9350: remove explicit memtracker from scannode
URL: https://github.com/apache/doris/pull/9350
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] dataroaring commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
dataroaring commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1118235552
> > _block_mem_tracker
>
> So why remove `counterpart`, `counterpart` is just for `_block_mem_tracker`.
It seems that _block_mem_tracker tracks memory for vectorized block? If so, we should track them via ExecNode::_mem_tracker， and it should be tracked via hook, right? btw, we should track mmap allocated memory.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] dataroaring commented on pull request #9350: remove explicit memtracker from scannode
Posted by GitBox <gi...@apache.org>.
dataroaring commented on PR #9350:
URL: https://github.com/apache/incubator-doris/pull/9350#issuecomment-1118248438
> > > > _block_mem_tracker
> > >
> > >
> > > So why remove `counterpart`, `counterpart` is just for `_block_mem_tracker`.
> >
> >
> > It seems that _block_mem_tracker tracks memory for vectorized block? If so, we should track them via ExecNode::_mem_tracker， and it should be tracked via hook, right? btw, we should track mmap allocated memory.
>
> mmap has been tracked and introduced in pr: #9145
>
> `_block_mem_tracker` only tracks memory for vectorized block, `ExecNode::_mem_tracker` includes other memory in ExecNode in addition to memory for vectorized block. This is the purpose of my previous design, so my reply above is whether it is necessary.
Yep. I understand what you mean. IMHO, It should be case by case, e.g. for the OlapScanNode, the majority memory should be contributed by blocks, so value of block_mem_tracker is about equal value of ExecNode::_mem_tracker？ But for the AggregateNode, may be the majority memory should be contributed by hash table, so we should track memory usage of hash table.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org