Exchange receiver decode optimization to do squashing work at the same time by yibin87 · Pull Request #6202 · pingcap/tiflash

yibin87 · 2022-10-28T01:30:49Z

What problem does this PR solve?

Issue Number: close #6157

Problem Summary:

What is changed and how it works?

Previously, TiFlash adds a Squash transform after ExchangeReceiver, because the block output by ExchangeReceiver might be too small and not efficient to handle. However, the two-stage solution would introduce too many memory operations, like column allocations and de-allocations. So in this PR, the one-stage solution is provided.

For a simple local test, with 10 integer columns, 1024 rows per block, performance improves from: 140ms => 60ms.

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: yibin <huyibin@pingcap.com>

ti-chi-bot · 2022-10-28T01:30:50Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise
windtalker

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.

Details

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

Signed-off-by: yibin <huyibin@pingcap.com>

fuzhe1989 · 2022-10-28T01:43:24Z

Fine-grained partitioning will exacerbate the chunk fragmentation in exchange and would benefit from this pr much more from normal cases.

yibin87 · 2022-10-28T02:26:56Z

Fine-grained partitioning will exacerbate the chunk fragmentation in exchange and would benefit from this pr much more from normal cases.

Yeah, and for large-scale clusters, it would benefit also.

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

dbms/src/DataStreams/TiRemoteBlockInputStream.h

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

dbms/src/DataStreams/TiRemoteBlockInputStream.h

windtalker · 2022-10-28T10:14:13Z

dbms/src/Flash/Coprocessor/IChunkDecodeAndSquash.cpp

+            codec.readColumnMeta(i, istr, column);
+
+            /// Data
+            MutableColumnPtr read_column = column.type->createColumn();


Looks like read_column is not used?

Ah, removed it now.

windtalker · 2022-11-01T03:30:21Z

dbms/src/Flash/Coprocessor/tests/gtest_ti_remote_block_inputstream.cpp

+
+#include <Flash/Coprocessor/StreamingDAGResponseWriter.cpp>
+#include <Flash/Mpp/BroadcastOrPassThroughWriter.cpp>
+#include <Flash/Mpp/ExchangeReceiver.cpp>


format the includes

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2022-11-01T08:29:56Z

/run-unit-tests

Signed-off-by: yibin <huyibin@pingcap.com>

dbms/src/Flash/Coprocessor/IChunkDecodeAndSquash.cpp

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

dbms/src/DataStreams/TiRemoteBlockInputStream.h

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2022-11-02T02:30:07Z

/run-unit-tests

yibin87 · 2022-11-02T02:30:22Z

/run-integration-test

windtalker · 2022-11-02T02:41:40Z

dbms/src/Flash/Coprocessor/CHBlockChunkCodec.cpp

+        /// Data
+        MutableColumnPtr read_column = column.type->createColumn();
+        if (reserve_size > 0)
+            read_column->reserve(reserve_size);


reserve rows if reserve_size <= 0 and if reserve_size >0, reserve std::max(rows, reserve_size) ?

Make sense, Done.

Signed-off-by: yibin <huyibin@pingcap.com>

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

dbms/src/DataStreams/TiRemoteBlockInputStream.h

dbms/src/Flash/Coprocessor/IChunkDecodeAndSquash.cpp

Signed-off-by: yibin <huyibin@pingcap.com>

SeaRise

others LGTM

dbms/src/Flash/Coprocessor/CodecUtils.cpp

Signed-off-by: yibin <huyibin@pingcap.com>

SeaRise

LGTM

windtalker

LGTM

yibin87 · 2022-11-02T08:32:16Z

/merge

ti-chi-bot · 2022-11-02T08:32:18Z

@yibin87: 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.

Details

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-11-02T08:32:20Z

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

Details

Commit hash: e8594cf

JasonWu0506 · 2023-05-30T06:49:46Z

/type ehencement

ti-chi-bot · 2023-05-30T06:49:47Z

@JasonWu0506: The label(s) type/ehencement cannot be applied, because the repository doesn't have them.

Details

In response to this:

/type ehencement

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.

JasonWu0506 · 2023-05-30T06:50:07Z

/type enhencement

ti-chi-bot · 2023-05-30T06:50:09Z

@JasonWu0506: The label(s) type/enhencement cannot be applied, because the repository doesn't have them.

Details

In response to this:

/type enhencement

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.

yibin87 added 3 commits October 21, 2022 17:38

Implement decode in CHBlockChunkCodec class

38941c9

Signed-off-by: yibin <huyibin@pingcap.com>

Opt TiRemoteBlockInputStream decode logic with squashing

56b8e9e

Signed-off-by: yibin <huyibin@pingcap.com>

Add TiRemoteBlockInputstream gtest

7aa9f5b

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 28, 2022

yibin87 added 2 commits October 28, 2022 09:34

Remove squash transformation from TiRemoteReadeBlockInputStream

28b9840

Signed-off-by: yibin <huyibin@pingcap.com>

Fix a little issue

3174eab

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested review from SeaRise, guo-shaoge and windtalker October 28, 2022 01:38

SeaRise reviewed Oct 31, 2022

View reviewed changes

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

windtalker reviewed Nov 1, 2022

View reviewed changes

Merge two while loop and some changes to comments

b2d37b7

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested review from SeaRise and windtalker November 1, 2022 08:29

Fix comment

33ee23b

Signed-off-by: yibin <huyibin@pingcap.com>

SeaRise reviewed Nov 1, 2022

View reviewed changes

Changes to comments

f924e62

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested a review from SeaRise November 2, 2022 02:23

windtalker reviewed Nov 2, 2022

View reviewed changes

Change reserve size logic

6e3fa2b

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested a review from windtalker November 2, 2022 04:40

Format gtest header files

e253bbb

Signed-off-by: yibin <huyibin@pingcap.com>

Fix format issue

4f425fb

Signed-off-by: yibin <huyibin@pingcap.com>

SeaRise reviewed Nov 2, 2022

View reviewed changes

dbms/src/Flash/Mpp/ExchangeReceiver.cpp Outdated Show resolved Hide resolved

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

dbms/src/Flash/Coprocessor/IChunkDecodeAndSquash.cpp Outdated Show resolved Hide resolved

Refact a little

23a7d92

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested a review from SeaRise November 2, 2022 07:00

SeaRise reviewed Nov 2, 2022

View reviewed changes

dbms/src/Flash/Coprocessor/CodecUtils.cpp Outdated Show resolved Hide resolved

dbms/src/Flash/Coprocessor/CodecUtils.cpp Outdated Show resolved Hide resolved

Little update

e8594cf

Signed-off-by: yibin <huyibin@pingcap.com>

SeaRise approved these changes Nov 2, 2022

View reviewed changes

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

windtalker approved these changes Nov 2, 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 Nov 2, 2022

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

Merge branch 'master' into exchange_receiver_decode_opt

a32e3b3

ti-chi-bot merged commit dc28b51 into pingcap:master Nov 2, 2022

yibin87 mentioned this pull request Nov 2, 2022

Join & Aggregation Fine Grained Partition Optimization #6157

Closed

6 tasks

yibin87 mentioned this pull request Aug 3, 2023

Expand the functionality of Local Runtime Filter #7891

Open

6 tasks

Conversation

yibin87 commented Oct 28, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fuzhe1989 commented Oct 28, 2022

Uh oh!

yibin87 commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

windtalker Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

yibin87 Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

windtalker Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

yibin87 Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yibin87 commented Nov 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yibin87 commented Nov 2, 2022

Uh oh!

yibin87 commented Nov 2, 2022

Uh oh!

windtalker Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

yibin87 Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

yibin87 commented Nov 2, 2022

Uh oh!

ti-chi-bot commented Nov 2, 2022

Uh oh!

ti-chi-bot commented Nov 2, 2022

Uh oh!

JasonWu0506 commented May 30, 2023

Uh oh!

ti-chi-bot bot commented May 30, 2023

Uh oh!

JasonWu0506 commented May 30, 2023

Uh oh!

ti-chi-bot bot commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

ti-chi-bot commented Oct 28, 2022 •

edited

Loading

yibin87 commented Oct 28, 2022 •

edited

Loading