add support for auxiliary memory bound in mpmc_queue #6820

windtalker · 2023-02-15T02:39:31Z

What problem does this PR solve?

Issue Number: ref #6528

Problem Summary:
In TiFlash runtime, some executors(like ExchangeReceiver, MPPTunnelSender, UnionBlockInputStream) use a MPMC queue to buffer the data, but the queues are only limited by the number of elements. If the element occupies a large amount of memory, the buffer queue may occupy too much memory, this pr add auxiliary_memory_bound to mpmc queue, so the memory usage by the queue can be limited.

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2023-02-15T02:39:32Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise
gengliqi

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

dbms/src/Common/MPMCQueue.h

yibin87 · 2023-02-15T06:13:22Z

dbms/src/Common/MPMCQueue.h

@@ -436,6 +472,7 @@ class MPMCQueue
    Int64 write_pos = 0;
    Status status = Status::NORMAL;
    String cancel_reason;
+    size_t current_auxiliary_memory_usage = 0;


Use Int64 here to avoid incorrect get_auxiliary_memory_usage case?

gengliqi · 2023-02-15T06:34:06Z

dbms/src/Common/MPMCQueue.h

+        if (is_timeout)
+            return Result::TIMEOUT;


Suggested change

if (is_timeout)

return Result::TIMEOUT;

if constexpr (need_wait)

{

if (is_timeout)

return Result::TIMEOUT;

}

gengliqi · 2023-02-15T06:34:33Z

dbms/src/Common/MPMCQueue.h

+        if (is_timeout)
+            return Result::TIMEOUT;


gengliqi · 2023-02-15T06:47:21Z

dbms/src/Common/MPMCQueue.h

        }
        if (!isCancelled() && read_pos < write_pos)
        {
            auto & obj = getObj(read_pos);
            res = std::move(obj);
            destruct(obj);
+            current_auxiliary_memory_usage -= get_auxiliary_memory_usage(res);


It's a little wasteful to call get_auxiliary_memory_usage twice for one object.
And also it requires that these two return values of get_auxiliary_memory_usage must be the same.

OK, save auxiliary_memory in another vector.

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

SeaRise

Others LGTM

SeaRise · 2023-02-16T02:08:39Z

dbms/src/Common/MPMCQueue.h

+            current_auxiliary_memory_usage -= element_auxiliary_memory[pos % capacity];
+            element_auxiliary_memory[pos % capacity] = 0;


Suggested change

current_auxiliary_memory_usage -= element_auxiliary_memory[pos % capacity];

element_auxiliary_memory[pos % capacity] = 0;

auto & elem_value = element_auxiliary_memory[pos % capacity];

current_auxiliary_memory_usage -= elem_value;

elem_value = 0;

dbms/src/Common/MPMCQueue.h

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker · 2023-02-16T03:25:59Z

/merge

ti-chi-bot · 2023-02-16T03:26:00Z

@windtalker: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-02-16T03:26:02Z

This pull request has been accepted and is ready to merge.

Commit hash: aed9719

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2023

SeaRise self-requested a review February 15, 2023 02:43

SeaRise reviewed Feb 15, 2023

View reviewed changes

dbms/src/Common/MPMCQueue.h Show resolved Hide resolved

dbms/src/Common/MPMCQueue.h Outdated Show resolved Hide resolved

dbms/src/Common/MPMCQueue.h Outdated Show resolved Hide resolved

windtalker force-pushed the mpmc_queue_support_auxiliary_memory_bound branch from 7383faa to c021cb9 Compare February 15, 2023 03:29

yibin87 reviewed Feb 15, 2023

View reviewed changes

gengliqi reviewed Feb 15, 2023

View reviewed changes

windtalker force-pushed the mpmc_queue_support_auxiliary_memory_bound branch from c6db9b5 to 3f397ba Compare February 15, 2023 07:21

gengliqi approved these changes Feb 15, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 15, 2023

windtalker added 4 commits February 16, 2023 09:23

add support for auxiliary memory bound in mpmc_queue

ec411af

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

4f0636f

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

20b17ce

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

1800008

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

SeaRise approved these changes Feb 16, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 16, 2023

address comments

aed9719

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker force-pushed the mpmc_queue_support_auxiliary_memory_bound branch from 8b77e85 to aed9719 Compare February 16, 2023 03:24

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 16, 2023

ti-chi-bot merged commit f2abf0c into pingcap:master Feb 16, 2023

windtalker deleted the mpmc_queue_support_auxiliary_memory_bound branch May 6, 2023 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for auxiliary memory bound in mpmc_queue #6820

add support for auxiliary memory bound in mpmc_queue #6820

windtalker commented Feb 15, 2023

ti-chi-bot commented Feb 15, 2023 •

edited

Loading

yibin87 Feb 15, 2023

windtalker Feb 15, 2023

gengliqi Feb 15, 2023

gengliqi Feb 15, 2023

gengliqi Feb 15, 2023

windtalker Feb 15, 2023

SeaRise left a comment

SeaRise Feb 16, 2023

windtalker commented Feb 16, 2023

ti-chi-bot commented Feb 16, 2023

ti-chi-bot commented Feb 16, 2023

		current_auxiliary_memory_usage -= element_auxiliary_memory[pos % capacity];
		element_auxiliary_memory[pos % capacity] = 0;

-            current_auxiliary_memory_usage -= element_auxiliary_memory[pos % capacity];
-            element_auxiliary_memory[pos % capacity] = 0;
+            auto & elem_value = element_auxiliary_memory[pos % capacity];
+            current_auxiliary_memory_usage -= elem_value;
+            elem_value = 0;

add support for auxiliary memory bound in mpmc_queue #6820

add support for auxiliary memory bound in mpmc_queue #6820

Conversation

windtalker commented Feb 15, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 15, 2023 • edited Loading

yibin87 Feb 15, 2023

Choose a reason for hiding this comment

windtalker Feb 15, 2023

Choose a reason for hiding this comment

gengliqi Feb 15, 2023

Choose a reason for hiding this comment

gengliqi Feb 15, 2023

Choose a reason for hiding this comment

gengliqi Feb 15, 2023

Choose a reason for hiding this comment

windtalker Feb 15, 2023

Choose a reason for hiding this comment

SeaRise left a comment

Choose a reason for hiding this comment

SeaRise Feb 16, 2023

Choose a reason for hiding this comment

windtalker commented Feb 16, 2023

ti-chi-bot commented Feb 16, 2023

ti-chi-bot commented Feb 16, 2023

ti-chi-bot commented Feb 15, 2023 •

edited

Loading