Pipeline: use notify instead of polling for `ExchangeReceiver` #9073

SeaRise · 2024-05-22T16:32:10Z

What problem does this PR solve?

Issue Number: ref #8869

Problem Summary:

What is changed and how it works?

The cpu usage of wait reactor is reduced to 0 in tpch50.

Check List

Tests

Tested tpch50, no performance regression

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: Lloyd-Pottiger <[email protected]>

windtalker · 2024-09-02T07:45:16Z

dbms/src/Flash/Coprocessor/DAGContext.h

@@ -374,6 +374,8 @@ class DAGContext
    /* const */ bool is_disaggregated_task = false; // a disagg task handling by the write node
    // `tunnel_set` is always set by `MPPTask` and is used later.
    MPPTunnelSetPtr tunnel_set;
+    // `mpp_receiver_set` is always set by `MPPTask` and is used later.


why move it to here

https://github.com/pingcap/tiflash/pull/9073/files#diff-0878026e4933aaca8d4180924369d51711e37f6093444231bf1665481d93b068R186-R187
Because it needs to be public

It can be solved by adding a public function?

A public function can be added here, but since mpp-tunnel-set is already in the public section, it might be better to maintain consistency.

But it will change the destructor order in DAGContext, I'm not sure if there is some potential risk of this.

ok, updated.

windtalker · 2024-09-04T01:15:03Z

dbms/src/Flash/Mpp/ReceivedMessageQueue.cpp

@@ -152,6 +150,15 @@ MPMCQueueResult ReceivedMessageQueue::pop(size_t stream_id, ReceivedMessagePtr &
                grpc_recv_queue.tryDequeue();
 #endif
            }
+            ExchangeReceiverMetric::subDataSizeMetric(*data_size_in_queue, recv_msg->getPacket().ByteSizeLong());


Should be a bug in the previous implementation, this subDataSizeMetric should only be called if recv_msg->getRemainingConsumers()->fetch_sub(1) == 1

ok, updated.

windtalker · 2024-09-04T01:29:51Z

dbms/src/Flash/Pipeline/Schedule/Tasks/NotifyFuture.h


-extern thread_local NotifyFuturePtr current_notify_future;
+extern thread_local NotifyFuture * current_notify_future;


Looks like this change may increase the risk of using a object that have been already released, is it by design that the current_notify_future will not be released during its lifecycle?

yes, current_notify_future is only released when the pipeline task changes from wait-for-notify to running

windtalker · 2024-09-20T05:31:06Z

dbms/src/Flash/Mpp/ReceivedMessageQueue.h

+        : queue(log_, std::forward<Args>(args)...)
+    {}
+
+    void registerTask(TaskPtr && task) override { queue.registerPipeReadTask(std::move(task)); }


maybe rename it to registerPipelineReadTask?

Because this is an override of NotifyFuture::registerTask, it cannot be renamed

windtalker

lgtm

gengliqi · 2024-09-27T08:20:58Z

dbms/src/Flash/Mpp/ReceivedMessageQueue.h

+private:
+    using QueueImpl = LooseBoundedMPMCQueue<ReceivedMessagePtr>;
+    // these are unbounded queues.
+    std::unique_ptr<QueueImpl> queue = std::make_unique<QueueImpl>(std::numeric_limits<size_t>::max());


Why use unique_ptr? Also, how about letting MSGChannel inherits LooseBoundedMPMCQueue<ReceivedMessagePtr>?

ref #7963,
Because LooseBoundedMPMCQueue includes locks, it must use unique_ptr or shared_ptr somewhere.

gengliqi · 2024-09-27T08:24:37Z

dbms/src/Flash/Mpp/ReceivedMessageQueue.h

+    void registerPipeWriteTask(TaskPtr && task) { queue.registerPipeWriteTask(std::move(task)); }
+
+private:
+    GRPCRecvQueue<ReceivedMessagePtr> queue;


How about directly letting GRPCRecvNotifyQueue inherit GRPCRecvQueue<ReceivedMessagePtr>? If so, these functions do not need to be written, only the registerTask function is required.

Signed-off-by: gengliqi <[email protected]>

gengliqi

LGTM

ti-chi-bot · 2024-09-27T10:48:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gengliqi, windtalker

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gengliqi,windtalker]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-09-27T10:48:38Z

[LGTM Timeline notifier]

Timeline:

2024-09-23 03:00:55.990452415 +0000 UTC m=+1448525.730876363: ☑️ agreed by windtalker.
2024-09-27 10:48:38.150357477 +0000 UTC m=+6273.570570488: ☑️ agreed by gengliqi.

SeaRise · 2024-09-27T10:53:32Z

/retest

Signed-off-by: gengliqi <[email protected]>

gengliqi · 2024-09-27T13:51:02Z

/retest

SeaRise added 8 commits May 22, 2024 18:02

add notify_future for mpptunnel

fa590d8

add for writer

62d3add

for exchange sender

e7d8c9b

remove writable

2ffee76

Merge branch 'master' into notify_for_sender

f76b7bd

try fmt

563573b

refine ut

e1e920c

for receiver

2f258ae

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 22, 2024

SeaRise added 2 commits May 23, 2024 00:33

u

f8e1dc6

fix

e2adf5e

SeaRise changed the title ~~Pipeline: use notify instead of polling for ExchangeReceiver~~ DNM: Pipeline: use notify instead of polling for ExchangeReceiver May 22, 2024

SeaRise mentioned this pull request May 23, 2024

Use notify instead of polling in pipeline model #8869

Closed

8 tasks

Lloyd-Pottiger and others added 16 commits May 23, 2024 14:33

format

c5655c6

Signed-off-by: Lloyd-Pottiger <[email protected]>

Update gtest_mpptunnel.cpp

da81c3c

Merge branch 'master' into notify_for_sender

249cfa9

Merge branch 'master' into notify_for_receiver

2c12e6c

fix comment

03dd7b6

Merge branch 'master' into notify_for_sender

d2814b8

Merge branch 'master' into notify_for_receiver

c0d784e

Merge branch 'master' into notify_for_sender

d9640df

fix may hang issue

df23506

some fix

0c43a11

fix ut

94d954b

refine err msg

dd1316f

Merge branch 'notify_for_sender' into notify_for_receiver

22ea753

add cancel

902291b

Merge branch 'master' into notify_for_sender

4ddcf98

Merge branch 'master' into notify_for_receiver

95fdb7f

windtalker reviewed Sep 4, 2024

View reviewed changes

Update ReceivedMessageQueue.cpp

bd66511

SeaRise requested a review from windtalker September 4, 2024 02:52

windtalker mentioned this pull request Sep 9, 2024

Fix TiFlash hang issue after #9072 #9424

Merged

12 tasks

SeaRise added 2 commits September 18, 2024 16:39

Update DAGContext.h

d13644d

Update PipelineExecutorContext.cpp

1f2423f

windtalker reviewed Sep 20, 2024

View reviewed changes

SeaRise requested a review from windtalker September 20, 2024 06:32

windtalker approved these changes Sep 23, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 23, 2024

Merge branch 'master' into notify_for_receiver

8da6615

gengliqi reviewed Sep 27, 2024

View reviewed changes

gengliqi added 2 commits September 27, 2024 17:43

Merge branch 'master' into notify_for_receiver

b38899f

update

c6033a8

Signed-off-by: gengliqi <[email protected]>

ti-chi-bot 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 Sep 27, 2024

update

bb18d78

Signed-off-by: gengliqi <[email protected]>

gengliqi force-pushed the notify_for_receiver branch from fa91f69 to bb18d78 Compare September 27, 2024 10:41

update name

c85801b

Signed-off-by: gengliqi <[email protected]>

gengliqi approved these changes Sep 27, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 27, 2024

Merge branch 'master' into notify_for_receiver

275134b

fix

af512d3

Signed-off-by: gengliqi <[email protected]>

ti-chi-bot bot merged commit 92e8dac into pingcap:master Sep 27, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pipeline: use notify instead of polling for `ExchangeReceiver` #9073

Pipeline: use notify instead of polling for `ExchangeReceiver` #9073

SeaRise commented May 22, 2024 •

edited

Loading

windtalker Sep 2, 2024

SeaRise Sep 4, 2024

windtalker Sep 4, 2024

SeaRise Sep 4, 2024

windtalker Sep 18, 2024

SeaRise Sep 18, 2024

windtalker Sep 4, 2024

SeaRise Sep 4, 2024

windtalker Sep 4, 2024

SeaRise Sep 4, 2024

windtalker Sep 20, 2024

SeaRise Sep 20, 2024

windtalker left a comment

gengliqi Sep 27, 2024

SeaRise Sep 27, 2024

gengliqi Sep 27, 2024

gengliqi left a comment

ti-chi-bot bot commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

SeaRise commented Sep 27, 2024

gengliqi commented Sep 27, 2024


		extern thread_local NotifyFuturePtr current_notify_future;
		extern thread_local NotifyFuture * current_notify_future;

Pipeline: use notify instead of polling for ExchangeReceiver #9073

Pipeline: use notify instead of polling for ExchangeReceiver #9073

Conversation

SeaRise commented May 22, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

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

Choose a reason for hiding this comment

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

gengliqi left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

[LGTM Timeline notifier]

SeaRise commented Sep 27, 2024

gengliqi commented Sep 27, 2024

Pipeline: use notify instead of polling for `ExchangeReceiver` #9073

Pipeline: use notify instead of polling for `ExchangeReceiver` #9073

SeaRise commented May 22, 2024 •

edited

Loading