[Runtime][Pipeline Executor] multiple threads management and the data forwarding notification mechanism. #10234

huajsj · 2022-02-13T06:12:08Z

RFC: https://github.com/apache/tvm-rfcs/blob/main/rfcs/0014-pipeline-executor.md
Tracking issue: #8596

In this patch we create multiple working threads for each runtime of pipeline. the threads would be terminated after the runtime class gets destroyed.

We also add a notification mechanism derived from the 'binding configuration' of the runtime to forward the data notification .

huajsj · 2022-02-14T18:25:11Z

@comaniac , @masahi , please take a look.

masahi · 2022-02-15T05:49:28Z

Thanks @huajsj. This is interesting and tricky code, I'll try review carefully. cc @comaniac

Where does this PR sit in the feature road map?

python/tvm/contrib/pipeline_executor.py

masahi · 2022-02-15T05:52:18Z

python/tvm/contrib/pipeline_executor.py

+        count : int
+            The times of pipeline running.
+        """
+        return self._get_pipe_execute_number()


You are using the term time or number throughout the code, but please use more specific terms. I think time is more like duration and number is more like elapsed_time?

Please update your naming convention in all files. I won't point out each of them.

I think num_executing_pipeline is better.

src/runtime/pipeline/pipeline_executor.cc

src/runtime/pipeline/pipeline_scheduler.cc

masahi · 2022-02-15T05:54:59Z

src/runtime/pipeline/pipeline_struct.h

+  /*!\brief Whether a data is ready or not.*/
+  volatile bool data_ready_ = false;
+  /*!\brief Whether the thread should exit or not.*/
+  volatile bool exit_state_ = false;


seems like not need to use atomic, because 'data_ready_' already protected by 'mutex_' and for exit_state, there is only a single 'writing' in ExitNotify then no race condition can happen. although atomic is very light weight but it still involved additional perf cost, if not necessary we may need to avoid use it here.

seems like not need to use atomic, because 'data_ready_' already protected by 'mutex_' and for exit_state, there is only a single 'writing' in ExitNotify then no race condition can happen

Then why do you need volatile? I'm not really familiar with volatile except that it is generally considered as a bad practice.

I agree with that volatile is bad practice for 'atomic' guarantee, but in our case we only need to guarantee the data 'visibility' under multiple thread mode , for such requirement volatile should be ok.

about 'atomic' guarantee part, for vairable 'data_ready_' the existing 'mutex_' and conditional variable will help for the atomic guarantee, for variable exit_state_, because each notification only have one parent and one child, under such single consumer and single producer scenario the both reading and writing of 'exit_state' are native atomic then no need additional protection.

I don't know, but all of SO answers I looked at say volatile is not needed for threading purpose. For example https://stackoverflow.com/questions/4557979/when-to-use-volatile-with-multi-threading

Any thought?

volatile has nothing to do with multi-threading or memory fence. it's only used in modern c++ to enforce reading from memory and disallow compilers to put the specific data from the register. Note that no memory fence is required to be introduced by volatile, so it doesn't officially guarantee anything about visibility. The rule of thumb is to never use volatile unless for usecases like DMA

@huajsj what do you think about using something like std::promise instead? while I think the use of volatile here is probably okay in a consumer-producer relation, i'm wondering if we can use a more established primitive to enable more folks in the community to work on the code.

@masahi ,@junrushao1994, @areusch , thanks for the follow up, I agree with that the promise is a better primitive and memory fence also be another issue we need to correct handle, after review code, I just realized there is still a memory barrier need to put after set 'exit_state = true' and before 'notify()' to guarantee the code 'ordering' , for such issue I may either use std::atomic or use 'promise + release barrier' and seems like std::atomic should be the better solution with one logic.

in the latest change I removed volatile for data_ready_ because mutex already did the acquire/release memory barrier, and change exit_state_ into a std::atomic, please check.

src/runtime/pipeline/pipeline_struct.h

huajsj · 2022-02-15T06:22:57Z

Thanks @huajsj. This is interesting and tricky code, I'll try review carefully. cc @comaniac

Where does this PR sit in the feature road map?

Thanks @masahi , this PR is part of RFC https://github.com/apache/tvm-rfcs/blob/main/rfcs/0014-pipeline-executor.md, the tracking issue is #8596.

data forwarding notification mechanism. In this patch we create working threads for each runtime of pipeline. the threads would be terminated once the runtime class gets destroyed. We also add a notification mechanism derived from the 'binding configuration' of the runtime to forward the data notification.

huajsj · 2022-02-17T23:05:57Z

@masahi, all review comments get addressed, please take a look.

src/runtime/pipeline/pipeline_executor.cc

leeexyz

Thanks for your efforts. Hope I can try this excellent feature very soon. :)

src/runtime/pipeline/pipeline_scheduler.cc

src/runtime/pipeline/pipeline_executor.h

src/runtime/pipeline/pipeline_struct.h

masahi · 2022-02-24T11:10:04Z

src/runtime/pipeline/pipeline_struct.h

+  /*\brief A map including the runtime input index and the notification data structure.*/
+  std::unordered_map<int, std::shared_ptr<DataNotify>> parents_notify_;
+  /*\brief The times of using pipeline function. */
+  uint32_t statistic_pipeline_execute_times_ = 0;


This variable names and its doc does not make a lot of sense ("the times of ..." is not really a word). In the next PR please improve them.

sure, will do that, thanks @masahi.

… forwarding notification mechanism. (apache#10234) * [Runtime][Pipeline Executor] multiple threads management and the data forwarding notification mechanism. In this patch we create working threads for each runtime of pipeline. the threads would be terminated once the runtime class gets destroyed. We also add a notification mechanism derived from the 'binding configuration' of the runtime to forward the data notification. * address review comments. * address review comments. * fix typo. * fix typo. * trigger build. * address review comments. * address review comments. * address review comments. * address review comments.

huajsj requested review from vinx13, tqchen, liangfu, areusch, tmoreau89, masahi, kazum, ZihengJiang, junrushao, comaniac, jroesch, yzhliu, merrymercy, icemelon and zhiics as code owners February 13, 2022 06:12

masahi reviewed Feb 15, 2022

View reviewed changes

python/tvm/contrib/pipeline_executor.py Outdated Show resolved Hide resolved

huajsj mentioned this pull request Feb 15, 2022

[RFC][Tracking Issue] Pipeline Executor For Compute graph pipeline #8596

Closed

15 tasks

masahi requested changes Feb 15, 2022

View reviewed changes

huajsj requested a review from masahi February 15, 2022 19:21

huajsj added 6 commits February 16, 2022 12:01

address review comments.

3545898

address review comments.

c9e353d

fix typo.

3f3b470

fix typo.

9392b2e

trigger build.

70f1560

huajsj force-pushed the MT branch from ad5b39a to 70f1560 Compare February 16, 2022 20:05

masahi reviewed Feb 21, 2022

View reviewed changes

src/runtime/pipeline/pipeline_executor.cc Outdated Show resolved Hide resolved

huajsj added 2 commits February 22, 2022 21:19

address review comments.

2d97c17

address review comments.

f1f4ca5

leeexyz reviewed Feb 23, 2022

View reviewed changes

huajsj added 2 commits February 23, 2022 13:21

address review comments.

161631a

address review comments.

c9497b0

huajsj requested a review from masahi February 24, 2022 04:49

masahi reviewed Feb 24, 2022

View reviewed changes

masahi approved these changes Feb 24, 2022

View reviewed changes

masahi merged commit 4102ebf into apache:main Feb 24, 2022

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Runtime][Pipeline Executor] multiple threads management and the data forwarding notification mechanism. #10234

[Runtime][Pipeline Executor] multiple threads management and the data forwarding notification mechanism. #10234

huajsj commented Feb 13, 2022 •

edited

Loading

huajsj commented Feb 14, 2022

masahi commented Feb 15, 2022 •

edited

Loading

masahi Feb 15, 2022

huajsj Feb 15, 2022

masahi Feb 21, 2022

huajsj Feb 23, 2022

masahi Feb 15, 2022

huajsj Feb 15, 2022

masahi Feb 21, 2022

huajsj Feb 22, 2022

masahi Feb 23, 2022

junrushao Feb 23, 2022

areusch Feb 23, 2022

huajsj Feb 23, 2022

huajsj commented Feb 15, 2022

huajsj commented Feb 17, 2022

leeexyz left a comment

masahi Feb 24, 2022

huajsj Feb 24, 2022

[Runtime][Pipeline Executor] multiple threads management and the data forwarding notification mechanism. #10234

[Runtime][Pipeline Executor] multiple threads management and the data forwarding notification mechanism. #10234

Conversation

huajsj commented Feb 13, 2022 • edited Loading

huajsj commented Feb 14, 2022

masahi commented Feb 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huajsj commented Feb 15, 2022

huajsj commented Feb 17, 2022

leeexyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huajsj commented Feb 13, 2022 •

edited

Loading

masahi commented Feb 15, 2022 •

edited

Loading