Improve the performance of partition table in extreme case #4988

bestwoody · 2022-05-24T07:32:40Z

What problem does this PR solve?

Issue Number: close #4474

Problem Summary:
Improve the performance of partition table in extreme case

What is changed and how it works?

use Multiplex technique to balancely read underlying DMSegmentInputstream to avoid consume starvation.
design doc: https://pingcap.feishu.cn/wiki/wikcntp2B8GEXyFLSbIbAtMJzBG?useNewLarklet=1
benchmark result: https://pingcap.feishu.cn/wiki/wikcntp2B8GEXyFLSbIbAtMJzBG?useNewLarklet=1#0fWIDd

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

Signed-off-by: bestwoody <bestwoody@163.com>

ti-chi-bot · 2022-05-24T07:32:41Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

windtalker
yibin87

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/DataStreams/MultiplexInputStream.h

Signed-off-by: bestwoody <bestwoody@163.com>

…into multiplex_dmsegstream

dbms/src/DataStreams/MultiplexInputStream.h

windtalker · 2022-05-31T05:27:28Z

dbms/src/DataStreams/MultiplexInputStream.h

+            streams_queue_id = 0;
+
+        auto & q_ptr = streams_queue_by_partition[streams_queue_id];
+        auto & q = *q_ptr;


Why need q?

just make code easier to read&write... and to show it will not be a nullptr.

oh, I figure out, since it's a std::shared_ptr, use dereference tech to avoid triger atomic counter of std::shared_ptr.

Then why not merge L56 and L57 as
auto & q = *streams_queue_by_partition[stream_queue_id]?

windtalker · 2022-05-31T06:32:05Z

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

@@ -777,7 +780,10 @@ void DAGStorageInterpreter::buildLocalStreams(DAGPipeline & pipeline, size_t max
                throw;
            }
        }
-        pipeline.streams.insert(pipeline.streams.end(), current_pipeline.streams.begin(), current_pipeline.streams.end());
+    }
+    for (int i = 0; i < static_cast<int>(max_streams); i++)


I think we don't need to use MultiplexInputStream for non-partition table scan or partition table scan with only one partition.

I agree with you, the only trouble is the code will be more dirty when two cases are not in a universal code path.

…into multiplex_dmsegstream

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

dbms/src/DataStreams/MultiplexInputStream.h

…into multiplex_dmsegstream

yibin87

LGTM

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

windtalker · 2022-06-06T03:02:15Z

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

@@ -642,8 +646,7 @@ void DAGStorageInterpreter::buildLocalStreams(DAGPipeline & pipeline, size_t max
        if (region_num == 0)
            continue;
        /// calculate weighted max_streams for each partition, note at least 1 stream is needed for each partition
-        size_t current_max_streams = table_query_infos.size() == 1 ? max_streams : (max_streams * region_num + total_local_region_num - 1) / total_local_region_num;
-
+        size_t current_max_streams = max_streams;


Seems we don't need current_max_streams anymore, just use max_streams is enough.

windtalker · 2022-06-06T03:04:24Z

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

+    if (has_multiple_partitions)
+    {
+        String req_info = dag_context.isMPPTask() ? dag_context.getMPPTaskId().toString() : "";
+        for (int i = 0; i < static_cast<int>(max_streams); ++i)


The stream number should be std::min(max_streams, the stream number in stream_pool)?

yes, nice find

windtalker · 2022-06-06T03:07:23Z

dbms/src/DataStreams/MultiplexInputStream.h

+            streams_queue_id = 0;
+
+        auto & q_ptr = streams_queue_by_partition[streams_queue_id];
+        auto & q = *q_ptr;


Then why not merge L56 and L57 as
auto & q = *streams_queue_by_partition[stream_queue_id]?

…into multiplex_dmsegstream

fuzhe1989 · 2022-06-07T01:06:49Z

dbms/src/DataStreams/MultiplexInputStream.h

+            std::make_shared<std::queue<std::shared_ptr<IBlockInputStream>>>());
+        for (const auto & stream : cur_streams)
+            streams_queue_by_partition.back()->push(stream);


nit: you could construct the queue before entering the critical area.

auto queue = std::make_shared<std::queue<BlockInputStreamPtr>>(cur_streams.begin(), cur_streams.end()); std::unique_lock lk(mu);

fuzhe1989 · 2022-06-07T01:09:51Z

dbms/src/DataStreams/MultiplexInputStream.h

+        if (queue_id + 1 < static_cast<int>(streams_queue_by_partition.size()))
+            return queue_id + 1;
+        else
+            return 0;


Suggested change

if (queue_id + 1 < static_cast<int>(streams_queue_by_partition.size()))

return queue_id + 1;

else

return 0;

return (queue_id + 1) % streams_queue_by_partition.size();

dbms/src/DataStreams/MultiplexInputStream.h

fuzhe1989 · 2022-06-07T01:11:13Z

dbms/src/DataStreams/MultiplexInputStream.h

+        std::shared_ptr<std::queue<
+            std::shared_ptr<IBlockInputStream>>>>
+        streams_queue_by_partition;
+    std::vector<std::shared_ptr<IBlockInputStream>> added_streams;


nit: use BlockInputStreamPtr other than std::shared_ptr<IBlockInputStream> .

fuzhe1989 · 2022-06-07T01:13:19Z

dbms/src/DataStreams/MultiplexInputStream.h

+        std::shared_ptr<std::queue<
+            std::shared_ptr<IBlockInputStream>>>>


nit: according to ClickHouse's coding style, better to add an alias for std::queue<std::shared_ptr<IBlockInputStream>> like:

using BlockInputStreamPtrQueue = std::queue<std::shared_ptr<IBlockInputStream>>;

Co-authored-by: Fu Zhe <fuzhe1989@gmail.com>

windtalker

LGTM

bestwoody · 2022-06-08T06:10:10Z

/merge

ti-chi-bot · 2022-06-08T06:10:11Z

@bestwoody: 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 · 2022-06-08T06:10:13Z

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

Commit hash: b90332f

sre-bot · 2022-06-08T06:26:33Z

Coverage for changed files

Filename                                        Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DataStreams/MultiplexInputStream.h                   73                73     0.00%          16                16     0.00%         134               134     0.00%          48                48     0.00%
Flash/Coprocessor/DAGStorageInterpreter.cpp         436               436     0.00%          32                32     0.00%         883               883     0.00%         290               290     0.00%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                               509               509     0.00%          48                48     0.00%        1017              1017     0.00%         338               338     0.00%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18327      9657             47.31%    205617  96719        52.96%

full coverage report (for internal network access only)

bestwoody added 5 commits May 9, 2022 16:18

init multiplex dm seg inputstream

121e33a

Signed-off-by: bestwoody <bestwoody@163.com>

nealy finish

fcbbcd7

merge

2de7f7f

update

2ade0b0

Signed-off-by: bestwoody <bestwoody@163.com>

refine

4a3a80b

Signed-off-by: bestwoody <bestwoody@163.com>

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label May 24, 2022

Merge branch 'master' into multiplex_dmsegstream

b0a6054

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 24, 2022

bestwoody requested review from windtalker, fzhedu and yibin87 May 24, 2022 07:33

Merge branch 'master' into multiplex_dmsegstream

20d34c5

yibin87 reviewed May 27, 2022

View reviewed changes

dbms/src/DataStreams/MultiplexInputStream.h Outdated Show resolved Hide resolved

bestwoody added 2 commits May 30, 2022 11:59

update

f108794

Signed-off-by: bestwoody <bestwoody@163.com>

Merge branch 'multiplex_dmsegstream' of github.com:bestwoody/tiflash …

96c38bd

…into multiplex_dmsegstream

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 30, 2022

Merge branch 'master' into multiplex_dmsegstream

e8a6878

windtalker reviewed May 31, 2022

View reviewed changes

bestwoody and others added 6 commits May 31, 2022 15:57

refine

dd64aee

Merge branch 'multiplex_dmsegstream' of github.com:bestwoody/tiflash …

8498f5b

…into multiplex_dmsegstream

optimize no parition mode

3843ef3

refine

a77b5f7

slim

6e89cfa

Merge branch 'master' into multiplex_dmsegstream

a88005c

SeaRise reviewed May 31, 2022

View reviewed changes

dbms/src/DataStreams/MultiplexInputStream.h Outdated Show resolved Hide resolved

dbms/src/DataStreams/MultiplexInputStream.h Show resolved Hide resolved

bestwoody added 2 commits May 31, 2022 19:34

regine

db7c571

Merge branch 'multiplex_dmsegstream' of github.com:bestwoody/tiflash …

dc1a4f8

…into multiplex_dmsegstream

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 1, 2022

bestwoody and others added 2 commits June 1, 2022 12:06

add mpptask check

3c26f50

Merge branch 'master' into multiplex_dmsegstream

ce9414e

yibin87 approved these changes Jun 1, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 1, 2022

windtalker reviewed Jun 6, 2022

View reviewed changes

bestwoody added 2 commits June 6, 2022 15:38

refine code

f702979

Merge branch 'multiplex_dmsegstream' of github.com:bestwoody/tiflash …

3e47b2c

…into multiplex_dmsegstream

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2022

bestwoody and others added 3 commits June 6, 2022 15:40

code slim

0c215ed

Merge branch 'master' into multiplex_dmsegstream

ba4dc56

Merge branch 'master' into multiplex_dmsegstream

b3b80cd

fuzhe1989 reviewed Jun 7, 2022

View reviewed changes

Update dbms/src/DataStreams/MultiplexInputStream.h

d434dfb

Co-authored-by: Fu Zhe <fuzhe1989@gmail.com>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 7, 2022

windtalker approved these changes Jun 8, 2022

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 Jun 8, 2022

Merge branch 'master' into multiplex_dmsegstream

b90332f

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 8, 2022

ti-chi-bot merged commit 5b61ae7 into pingcap:master Jun 8, 2022

bestwoody deleted the multiplex_dmsegstream branch June 8, 2022 06:37

gengliqi mentioned this pull request Dec 12, 2023

TiFlash may be OOM when scanning many partition tables under stream model #8505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the performance of partition table in extreme case #4988

Improve the performance of partition table in extreme case #4988

bestwoody commented May 24, 2022 •

edited

Loading

ti-chi-bot commented May 24, 2022 •

edited

Loading

windtalker May 31, 2022

bestwoody May 31, 2022

bestwoody May 31, 2022

windtalker Jun 6, 2022

bestwoody Jun 6, 2022

bestwoody Jun 6, 2022

windtalker May 31, 2022

bestwoody May 31, 2022

bestwoody May 31, 2022

yibin87 left a comment

windtalker Jun 6, 2022

bestwoody Jun 6, 2022

windtalker Jun 6, 2022

bestwoody Jun 6, 2022

bestwoody Jun 6, 2022

windtalker Jun 6, 2022

fuzhe1989 Jun 7, 2022

fuzhe1989 Jun 7, 2022

fuzhe1989 Jun 7, 2022

fuzhe1989 Jun 7, 2022

windtalker left a comment

bestwoody commented Jun 8, 2022

ti-chi-bot commented Jun 8, 2022

ti-chi-bot commented Jun 8, 2022

sre-bot commented Jun 8, 2022

		std::shared_ptr<std::queue<
		std::shared_ptr<IBlockInputStream>>>>

Improve the performance of partition table in extreme case #4988

Improve the performance of partition table in extreme case #4988

Conversation

bestwoody commented May 24, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yibin87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

bestwoody commented Jun 8, 2022

ti-chi-bot commented Jun 8, 2022

ti-chi-bot commented Jun 8, 2022

sre-bot commented Jun 8, 2022

Coverage for changed files

Coverage summary

bestwoody commented May 24, 2022 •

edited

Loading

ti-chi-bot commented May 24, 2022 •

edited

Loading