change the exchange packets interface among tiflash #3184

fzhedu · 2021-10-08T12:16:15Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

exchange encodes blocks twice thus decodes blocks twice, i.e., block -> response -> packet , taking more CPU, to solve this issue, this PR directly serializes blocks into packets. In detail, block -> packet for the non-last packets, but for the last packet, block -> packet.chunks and execution_summaries -> response -> packet.data. This change aims to exchanging data among tiflash, sending data from tiflash to tidb as previous doing.

What is changed and how it works?

Proposal: change kvproto

What's Changed:

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch:

Check List

Tests

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

Side effects

Release note

reduce unnecessary memory copy when encoding/decoding packets for exchanging data among tiflash

ti-chi-bot · 2021-10-08T12:16:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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.

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

windtalker · 2021-10-09T05:32:55Z

dbms/src/Flash/Coprocessor/CHBlockChunkCodec.cpp

@@ -91,4 +93,14 @@ Block CHBlockChunkCodec::decode(const tipb::Chunk & chunk, const DAGSchema & sch
    return block_in.read();
 }

+Block CHBlockChunkCodec::decode(const std::string & str, const DAGSchema & schema)


L88-L93 is the same as L98-L103, should reuse them.

windtalker · 2021-10-09T05:34:48Z

dbms/src/Flash/Coprocessor/CHBlockChunkCodec.h

 class CHBlockChunkCodec : public ChunkCodec
 {
 public:
    CHBlockChunkCodec() = default;
    Block decode(const tipb::Chunk &, const DAGSchema &) override;
+    Block decode(const std::string &, const DAGSchema &);


Why not just change Block decode(const tipb::Chunk &, const DAGSchema &) to Block decode(const std::string &, const DAGSchema &) in the base class?

windtalker · 2021-10-09T05:40:02Z

dbms/src/Flash/Coprocessor/DAGQuerySource.h

-
+    void write(mpp::MPPDataPacket &)
+    {
+        assert(false);


Throw exception with unsupported error.

windtalker · 2021-10-09T05:40:12Z

dbms/src/Flash/Coprocessor/DAGQuerySource.h

+    }
+    void write(mpp::MPPDataPacket &, [[maybe_unused]] uint16_t)
+    {
+        assert(false);


windtalker · 2021-10-09T05:45:00Z

dbms/src/Flash/Coprocessor/StreamingDAGResponseWriter.cpp

    if (records_per_chunk == -1)
    {
+        mpp::MPPDataPacket packet;


TiSpark supports CHBlock encode type, will it be affected by this change?

writing to tispark uses current_memory_tracker? I suppose this branch only works for shuffing among tiflash.

What do you mean by writing to tispark uses current_memory_tracker? Writing to TiSpark does use StreamingDAGResponseWriter. And I think we can only guarantee that partitionAndEncodeThenWriteBlocks is used among TiFlash.

windtalker · 2021-10-09T07:30:41Z

dbms/src/DataStreams/TiRemoteBlockInputStream.h

+
+        auto rows = result.decodeChunks(block_queue, remote_reader->getOutputSchema(), expected_types);
+        /// return empty msg after all its chunks are decoded.
+        remote_reader->returnEmptyMsg(&result);


It looks weird to divide the fetch result into 3 part:

remote_reader->nextResult

result.decodeChunks

remote_reader->returnEmptyMsg

I suggest to redefine remote_reader->nextResult, move the logical of Chunk => Block into nextResult, so we do not need result.decodeChunks and remote_reader->returnEmptyMsg, just call remote_reader->nextResult is enough.

fzhedu · 2021-10-11T03:20:13Z

/run-all-tests

fzhedu · 2021-10-11T03:52:27Z

/run-all-tests

fzhedu · 2021-10-11T05:35:32Z

/run-all-tests

windtalker · 2021-10-11T05:50:58Z

dbms/src/Flash/Coprocessor/StreamingDAGResponseWriter.cpp

    if (records_per_chunk == -1)
    {
+        mpp::MPPDataPacket packet;


What do you mean by writing to tispark uses current_memory_tracker? Writing to TiSpark does use StreamingDAGResponseWriter. And I think we can only guarantee that partitionAndEncodeThenWriteBlocks is used among TiFlash.

windtalker · 2021-10-11T06:01:34Z

dbms/src/Flash/Mpp/MPPTunnelSet.cpp

+void MPPTunnelSetBase<Tunnel>::write(mpp::MPPDataPacket & packet)
+{
+    auto tunnels_size = tunnels.size();
+    if (tunnels_size > 1)


when tunnels_size == 1, the packet is ignored?

it is a bug, fixed.

windtalker · 2021-10-11T06:04:28Z

dbms/src/Flash/Coprocessor/StreamingDAGResponseWriter.cpp

-        responses[i] = response;
-        responses[i].set_encode_type(encode_type);
+        if constexpr (for_last_response)
+        {


For both broadcast and partition writer, only one response needs to carry the execution summary info.

updated in TunnelSet->write()

fzhedu · 2021-10-11T12:56:34Z

/run-all-tests

fzhedu · 2021-10-12T07:04:31Z

/run-all-tests

fzhedu · 2021-10-12T10:41:39Z

/run-all-tests

fzhedu · 2021-10-13T07:13:33Z

/run-all-tests

windtalker

others LGTM

windtalker · 2021-10-13T06:59:28Z

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

+            block_queue.push(std::move(block));
+        }
+    }
+    else if (!recv_msg->packet->data().empty())


useless code?

windtalker · 2021-10-13T09:24:37Z

dbms/src/Flash/Coprocessor/StreamingDAGResponseWriter.cpp

-        responses[i].set_encode_type(encode_type);
+        if constexpr (for_last_response)
+        {
+            if (i == 0)


Add some comments to make it more readable.

windtalker · 2021-10-13T09:25:32Z

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

@@ -1,5 +1,8 @@
 #include <Common/CPUAffinityManager.h>
 #include <Common/ThreadFactory.h>
+#include <Flash/Coprocessor/ArrowChunkCodec.h>
+#include <Flash/Coprocessor/CoprocessorReader.h>
+#include <Flash/Coprocessor/DefaultChunkCodec.h>


useless header file.

fzhedu · 2021-10-14T09:10:57Z

/merge

ti-chi-bot · 2021-10-14T09:10:58Z

@fzhedu: 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 · 2021-10-14T09:11:05Z

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

Commit hash: f5134d8

fzhedu requested review from windtalker and fuzhe1989 October 8, 2021 12:16

ti-chi-bot added release-note size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 8, 2021

fzhedu force-pushed the ReduceEncodeCopy branch from f59ea35 to 4dfd62f Compare October 8, 2021 12:22

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Oct 8, 2021

windtalker reviewed Oct 9, 2021

View reviewed changes

fzhedu force-pushed the ReduceEncodeCopy branch from 1fddfe6 to b6aeae4 Compare October 11, 2021 03:52

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2021

fzhedu requested a review from windtalker October 11, 2021 05:35

windtalker reviewed Oct 11, 2021

View reviewed changes

fzhedu requested a review from windtalker October 12, 2021 10:41

windtalker approved these changes Oct 13, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 13, 2021

fzhedu added 6 commits October 14, 2021 17:10

change exchange receiver packets

d9532cb

change exchange sender interface

903a9d4

address comments to change encode interface

4babd90

update receiver and add comments

5296d70

fix a bug

29a727a

address comments

c23d3ee

fzhedu added 3 commits October 14, 2021 17:10

TiFlash also receives data in response type if mocking it as TIDB

24a0f3f

updated encodechunks

fcf10e7

address comments

f5134d8

fzhedu force-pushed the ReduceEncodeCopy branch from 7415435 to f5134d8 Compare October 14, 2021 09:10

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 14, 2021

ti-chi-bot merged commit b7e87dd into pingcap:master Oct 14, 2021

fzhedu mentioned this pull request Oct 25, 2021

the execution summaris are not right #3284

Closed

windtalker mentioned this pull request Dec 31, 2021

TiFlash will crash if sending more than 2GB message through grpc #3785

Closed

JaySon-Huang deleted the ReduceEncodeCopy branch January 5, 2022 13:01

fzhedu restored the ReduceEncodeCopy branch March 25, 2022 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change the exchange packets interface among tiflash #3184

change the exchange packets interface among tiflash #3184

fzhedu commented Oct 8, 2021

ti-chi-bot commented Oct 8, 2021 •

edited

windtalker Oct 9, 2021

windtalker Oct 9, 2021

fzhedu Oct 11, 2021

windtalker Oct 9, 2021

fzhedu Oct 11, 2021

windtalker Oct 9, 2021

fzhedu Oct 11, 2021

windtalker Oct 9, 2021

fzhedu Oct 11, 2021

windtalker Oct 11, 2021

windtalker Oct 9, 2021

fzhedu commented Oct 11, 2021

fzhedu commented Oct 11, 2021

fzhedu commented Oct 11, 2021

windtalker Oct 11, 2021

windtalker Oct 11, 2021

fzhedu Oct 12, 2021

windtalker Oct 11, 2021

fzhedu Oct 13, 2021

fzhedu commented Oct 11, 2021

fzhedu commented Oct 12, 2021

fzhedu commented Oct 12, 2021

fzhedu commented Oct 13, 2021

windtalker left a comment

windtalker Oct 13, 2021

fzhedu Oct 14, 2021

windtalker Oct 13, 2021

fzhedu Oct 14, 2021

windtalker Oct 13, 2021

fzhedu Oct 14, 2021

fzhedu commented Oct 14, 2021

ti-chi-bot commented Oct 14, 2021

ti-chi-bot commented Oct 14, 2021

change the exchange packets interface among tiflash #3184

change the exchange packets interface among tiflash #3184

Conversation

fzhedu commented Oct 8, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Oct 8, 2021 • edited

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

fzhedu commented Oct 11, 2021

fzhedu commented Oct 11, 2021

fzhedu commented Oct 11, 2021

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

fzhedu commented Oct 11, 2021

fzhedu commented Oct 12, 2021

fzhedu commented Oct 12, 2021

fzhedu commented Oct 13, 2021

windtalker 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

fzhedu commented Oct 14, 2021

ti-chi-bot commented Oct 14, 2021

ti-chi-bot commented Oct 14, 2021

ti-chi-bot commented Oct 8, 2021 •

edited