[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some operators #41602

jacktengg · 2024-10-09T08:04:50Z

Proposed changes

Issue Number: close #xxx

doris-robot · 2024-10-09T08:04:56Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

jacktengg · 2024-10-09T08:05:22Z

run buildall

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-09T08:12:00Z

be/src/pipeline/exec/operator.cpp

 PipelineXLocalStateBase::PipelineXLocalStateBase(RuntimeState* state, OperatorXBase* parent)
-        : _num_rows_returned(0),
-          _rows_returned_counter(nullptr),
+        : _rows_returned_counter(nullptr),


warning: member initializer for '_rows_returned_counter' is redundant [modernize-use-default-member-init]

Suggested change

: _rows_returned_counter(nullptr),

: ,

jacktengg · 2024-10-09T08:58:05Z

run buildall

doris-robot · 2024-10-09T09:48:11Z

TeamCity be ut coverage result:
Function Coverage: 37.27% (9631/25839)
Line Coverage: 28.67% (79885/278658)
Region Coverage: 28.10% (41285/146945)
Branch Coverage: 24.72% (21034/85098)
Coverage Report: http://coverage.selectdb-in.cc/coverage/3fe3a78a697e6c02916f02d62d419cc25696226f_3fe3a78a697e6c02916f02d62d419cc25696226f/report/index.html

yiguolei · 2024-10-09T10:02:13Z

be/src/pipeline/exec/aggregation_sink_operator.cpp

                                   data.get_buffer_size_in_bytes() -
-                                   Base::_shared_state->mem_usage_record.used_in_state);
+                                   Base::_shared_state->mem_usage_record.used_in_state;
+                           Base::_mem_tracker->consume(arena_memory_usage);


base 里的这个memtracker 还有用么？

有了MemoryUsage和MemoryUsagePeak这两个counter，这个_mem_tracker感觉可以删掉了。

yiguolei · 2024-10-09T10:32:50Z

be/src/pipeline/exec/hashjoin_build_sink.cpp

+            local_state._memory_used_counter->set(local_state._mem_tracker->consumption());
+            local_state._peak_memory_usage_counter->set(
+                    local_state._mem_tracker->peak_consumption());
+            COUNTER_SET(local_state._build_blocks_memory_usage, (int64_t)new_block_mem_usage);


至少跟580 行一致

be/src/pipeline/exec/exchange_sink_operator.cpp

yiguolei · 2024-10-09T11:01:22Z

be/src/pipeline/exec/exchange_sink_buffer.cpp

        }
        if (request.block_holder->get_block()) {
+            _parent->memory_used_counter()->update(
+                    -request.block_holder->get_block()->ByteSizeLong());


broadcast 的时候，是shared ptr，一个block 在多个channel 共享的，你这么统计，结果很大。
先别统计broadcast的了。

yiguolei · 2024-10-09T11:34:01Z

be/src/pipeline/exec/exchange_sink_buffer.cpp

            RETURN_IF_ERROR(
                    BeExecVersionManager::check_be_exec_version(request.block->be_exec_version()));
        }
+        _parent->memory_used_counter()->update(request.block->ByteSizeLong());


我们 exchange sink operator：

先只统计非broad cast 方式的内存，统计方式跟现在这个PR的代码一样。

channel 内保存的那一个block的内存。 broadcast方式的正好没有。

对于broadcast 方式的内存，可能是通过那个holder，构造和析构函数里计算，或者直接使用holder limiter的那个值。

jacktengg · 2024-10-09T17:46:18Z

run buildall

doris-robot · 2024-10-09T19:18:06Z

TeamCity be ut coverage result:
Function Coverage: 37.29% (9636/25838)
Line Coverage: 28.67% (79883/278608)
Region Coverage: 28.08% (41312/147110)
Branch Coverage: 24.70% (21037/85154)
Coverage Report: http://coverage.selectdb-in.cc/coverage/1255d5a76e37f24d0201d9497666cf97c1bc00c4_1255d5a76e37f24d0201d9497666cf97c1bc00c4/report/index.html

jacktengg · 2024-10-10T01:28:36Z

run buildall

doris-robot · 2024-10-10T02:06:09Z

TeamCity be ut coverage result:
Function Coverage: 37.29% (9636/25838)
Line Coverage: 28.69% (79921/278609)
Region Coverage: 28.08% (41314/147111)
Branch Coverage: 24.71% (21041/85154)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ac364bb5ecbb954f335bb8c10cb0b54d590120a9_ac364bb5ecbb954f335bb8c10cb0b54d590120a9/report/index.html

jacktengg · 2024-10-10T02:55:55Z

run p0

yiguolei · 2024-10-10T04:51:10Z

be/src/pipeline/exec/aggregation_sink_operator.cpp

-    Base::_mem_tracker->consume(arena_memory_usage);
-    _serialize_key_arena_memory_usage->add(arena_memory_usage);
-    Base::_shared_state->mem_usage_record.used_in_arena = _agg_arena_pool->size();
+    int64_t arena_memory_usage = _agg_arena_pool->size();


lost agg hash table?

yiguolei · 2024-10-10T04:52:11Z

be/src/pipeline/exec/aggregation_sink_operator.h

    RuntimeProfile::Counter* _serialize_data_timer = nullptr;
    RuntimeProfile::Counter* _deserialize_data_timer = nullptr;
    RuntimeProfile::Counter* _max_row_size_counter = nullptr;
    RuntimeProfile::Counter* _hash_table_memory_usage = nullptr;


This counter is not updated?

yiguolei · 2024-10-10T04:55:31Z

be/src/pipeline/exec/exchange_sink_operator.cpp

            SCOPED_TIMER(local_state._split_block_hash_compute_timer);
-            RETURN_IF_ERROR(
-                    local_state._partitioner->do_partitioning(state, block, _mem_tracker.get()));
+            RETURN_IF_ERROR(local_state._partitioner->do_partitioning(state, block, nullptr));


why add a nullptr not remove this parameter?

yiguolei · 2024-10-10T04:57:40Z

be/src/pipeline/exec/operator.cpp


    if (auto rows = block->rows()) {
-        _num_rows_returned += rows;
+        COUNTER_UPDATE(_rows_returned_counter, rows);


这里统一给加了，然后每个operator 内部也在加，难道没有错吗？

yiguolei · 2024-10-10T04:59:35Z

be/src/pipeline/exec/aggregation_source_operator.cpp

            vectorized::Block::filter_block_internal(block, _shared_state->need_computes);
            if (auto rows = block->rows()) {
-                _num_rows_returned += rows;
+                add_num_rows_returned(rows);


这里也加，然后base 里也加？

yiguolei · 2024-10-10T05:00:02Z

be/src/pipeline/exec/partitioned_hash_join_sink_operator.cpp

            {
                SCOPED_TIMER(_partition_timer);
-                (void)_partitioner->do_partitioning(state, &sub_block, _mem_tracker.get());
+                (void)_partitioner->do_partitioning(state, &sub_block, nullptr);


这里的nullptr 有啥意义吗？

jacktengg · 2024-10-10T08:46:10Z

run buildall

github-actions · 2024-10-10T08:46:29Z

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

github-actions · 2024-10-10T08:46:32Z

PR approved by anyone and no changes requested.

github-actions · 2024-10-10T08:49:06Z

clang-tidy review says "All clean, LGTM! 👍"

doris-robot · 2024-10-10T10:11:19Z

TeamCity be ut coverage result:
Function Coverage: 37.40% (9659/25827)
Line Coverage: 28.67% (80126/279478)
Region Coverage: 28.09% (41425/147451)
Branch Coverage: 24.70% (21099/85418)
Coverage Report: http://coverage.selectdb-in.cc/coverage/f4bd3d8ff1bfbe778bd3aa4facf3cb924bae4669_f4bd3d8ff1bfbe778bd3aa4facf3cb924bae4669/report/index.html

jacktengg · 2024-10-10T14:00:25Z

run buildall

doris-robot · 2024-10-10T14:47:08Z

TeamCity be ut coverage result:
Function Coverage: 37.41% (9661/25827)
Line Coverage: 28.68% (80168/279478)
Region Coverage: 28.10% (41439/147449)
Branch Coverage: 24.71% (21109/85416)
Coverage Report: http://coverage.selectdb-in.cc/coverage/f4bd3d8ff1bfbe778bd3aa4facf3cb924bae4669_f4bd3d8ff1bfbe778bd3aa4facf3cb924bae4669/report/index.html

mrhhsg

lgtm

…operators (apache#41602) ## Proposed changes Issue Number: close #xxx

…operators (apache#41602)

…operators (#41602) (#41886) BP #41602

…ize when EOF. (#47312) ### What problem does this PR solve? #41602 EOF clears _instance_to_package_queue but does not update total_queue_size, causing incorrect judgments that rely on total_queue_size. UT ``` mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :2 mock transmit_blockv2 dest ins id :3 queue size : 6 each queue size : Instance: 2, queue size: 2 Instance: 1, queue size: 2 Instance: 3, queue size: 2 queue size : 6 // error size each queue size : Instance: 2, queue size: 0 Instance: 1, queue size: 2 Instance: 3, queue size: 2 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :3 mock transmit_blockv2 dest ins id :3 ```

…ize when EOF. (apache#47312) apache#41602 EOF clears _instance_to_package_queue but does not update total_queue_size, causing incorrect judgments that rely on total_queue_size. UT ``` mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :2 mock transmit_blockv2 dest ins id :3 queue size : 6 each queue size : Instance: 2, queue size: 2 Instance: 1, queue size: 2 Instance: 3, queue size: 2 queue size : 6 // error size each queue size : Instance: 2, queue size: 0 Instance: 1, queue size: 2 Instance: 3, queue size: 2 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :3 mock transmit_blockv2 dest ins id :3 ```

…ize when EOF. (#47312) (#47621) #41602 EOF clears _instance_to_package_queue but does not update total_queue_size, causing incorrect judgments that rely on total_queue_size. UT ``` mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :2 mock transmit_blockv2 dest ins id :3 queue size : 6 each queue size : Instance: 2, queue size: 2 Instance: 1, queue size: 2 Instance: 3, queue size: 2 queue size : 6 // error size each queue size : Instance: 2, queue size: 0 Instance: 1, queue size: 2 Instance: 3, queue size: 2 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :3 mock transmit_blockv2 dest ins id :3 ``` ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test  - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason  - Behavior changed: - [ ] No. - [ ] Yes.  - Does this need documentation? - [ ] No. - [ ] Yes.  ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label  Co-authored-by: Mryange <yanxuecheng@selectdb.com>

…ize when EOF. (#47322) ### What problem does this PR solve? pick part from #47312 #41602 EOF clears _instance_to_package_queue but does not update total_queue_size, causing incorrect judgments that rely on total_queue_size. UT ``` mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :2 mock transmit_blockv2 dest ins id :3 queue size : 6 each queue size : Instance: 2, queue size: 2 Instance: 1, queue size: 2 Instance: 3, queue size: 2 queue size : 6 // error size each queue size : Instance: 2, queue size: 0 Instance: 1, queue size: 2 Instance: 3, queue size: 2 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :3 mock transmit_blockv2 dest ins id :3 ```

…ize when EOF. (apache#47312) ### What problem does this PR solve? apache#41602 EOF clears _instance_to_package_queue but does not update total_queue_size, causing incorrect judgments that rely on total_queue_size. UT ``` mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :2 mock transmit_blockv2 dest ins id :3 queue size : 6 each queue size : Instance: 2, queue size: 2 Instance: 1, queue size: 2 Instance: 3, queue size: 2 queue size : 6 // error size each queue size : Instance: 2, queue size: 0 Instance: 1, queue size: 2 Instance: 3, queue size: 2 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :1 mock transmit_blockv2 dest ins id :3 mock transmit_blockv2 dest ins id :3 ```

jacktengg force-pushed the fix-1009-counters branch from 5021271 to 5fa6ca1 Compare October 9, 2024 08:05

github-actions bot reviewed Oct 9, 2024

View reviewed changes

jacktengg force-pushed the fix-1009-counters branch 2 times, most recently from 69f6048 to 3fe3a78 Compare October 9, 2024 08:57

yiguolei reviewed Oct 9, 2024

View reviewed changes

be/src/pipeline/exec/exchange_sink_operator.cpp Outdated Show resolved Hide resolved

yiguolei reviewed Oct 9, 2024

View reviewed changes

jacktengg force-pushed the fix-1009-counters branch 3 times, most recently from 0b594f4 to 1255d5a Compare October 9, 2024 17:45

jacktengg force-pushed the fix-1009-counters branch 2 times, most recently from 275528a to ac364bb Compare October 10, 2024 01:28

yiguolei reviewed Oct 10, 2024

View reviewed changes

jacktengg force-pushed the fix-1009-counters branch from ac364bb to d9ef85e Compare October 10, 2024 08:36

github-actions bot added the approved Indicates a PR has been approved by one committer. label Oct 10, 2024

github-actions bot added the reviewed label Oct 10, 2024

mrhhsg approved these changes Oct 11, 2024

View reviewed changes

yiguolei merged commit 70f19d8 into apache:master Oct 11, 2024

yiguolei added dev/3.0.x not-merge/2.1 labels Oct 11, 2024

cjj2010 pushed a commit to cjj2010/doris that referenced this pull request Oct 12, 2024

[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some …

d3bf984

…operators (apache#41602) ## Proposed changes Issue Number: close #xxx

jacktengg added a commit to jacktengg/incubator-doris that referenced this pull request Oct 15, 2024

[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some …

024c373

…operators (apache#41602)

jacktengg mentioned this pull request Oct 15, 2024

[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some operators (#41602) #41886

Merged

jacktengg added a commit to jacktengg/incubator-doris that referenced this pull request Jan 2, 2025

[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some …

558fac6

…operators (apache#41602)

jacktengg added a commit to jacktengg/incubator-doris that referenced this pull request Jan 2, 2025

[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some …

1adef82

…operators (apache#41602)

yiguolei pushed a commit that referenced this pull request Jan 13, 2025

[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some …

1d52edc

…operators (#41602) (#41886) BP #41602

yiguolei added dev/3.0.4-merged and removed dev/3.0.x labels Jan 13, 2025

This was referenced Jan 22, 2025

[fix](exchange)fix exchange sink buffer does not update total_queue_size when EOF. #47312

Merged

[fix](exchange)fix exchange sink buffer does not update total_queue_size when EOF. #47322

Merged

jacktengg mentioned this pull request Feb 8, 2025

[fix](exchange)fix exchange sink buffer does not update total_queue_size when EOF. (#47312) #47621

Merged

16 tasks

[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some operators #41602

[fix](counters) fix MemoryUsage and PeakMemoryUsage counters of some operators #41602

Uh oh!

Conversation

jacktengg commented Oct 9, 2024

Proposed changes

Uh oh!

doris-robot commented Oct 9, 2024

Uh oh!

jacktengg commented Oct 9, 2024

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

jacktengg commented Oct 9, 2024

Uh oh!

doris-robot commented Oct 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacktengg Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacktengg commented Oct 9, 2024

Uh oh!

doris-robot commented Oct 9, 2024

Uh oh!

jacktengg commented Oct 10, 2024

Uh oh!

doris-robot commented Oct 10, 2024

Uh oh!

jacktengg commented Oct 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacktengg commented Oct 10, 2024

Uh oh!

github-actions bot commented Oct 10, 2024

Uh oh!

github-actions bot commented Oct 10, 2024

Uh oh!

github-actions bot commented Oct 10, 2024

Uh oh!

doris-robot commented Oct 10, 2024

Uh oh!

jacktengg commented Oct 10, 2024

Uh oh!

doris-robot commented Oct 10, 2024

Uh oh!

mrhhsg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jacktengg Oct 9, 2024 •

edited

Loading