remove explicit memtracker from scannode

[dataroaring]

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 by explaining why you chose the solution you did and what alternatives you considered, etc...

[morningman]

LGTM

[github-actions]

PR approved by at least one committer and no changes requested.

[github-actions]

PR approved by anyone and no changes requested.

[xinyiZzz]

_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

[dataroaring]

_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.

[xinyiZzz]

_block_mem_tracker

So why remove counterpart, counterpart is just for _block_mem_tracker.

[dataroaring]

_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.

[xinyiZzz]

_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.

[dataroaring]

_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.

[xinyiZzz]

_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.

[dataroaring]

_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

[xinyiZzz]

LGTM

[github-actions]

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!