[PATCH] D148728: [Support][Parallel] Add sequential mode to TaskGroup::spawn().

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/lib/Support/Parallel.cpp:16
 #include <future>
-#include <stack>
+#include <queue>
 #include <thread>
----------------
andrewng wrote:
> Unfortunately, at least in my testing on Windows, this change causes a drop in performance of around 3-4%. I believe that's why `std::stack` is used.
> Unfortunately, at least in my testing on Windows, this change causes a drop in performance of around 3-4%. I believe that's why `std::stack` is used.

will check it.


================
Comment at: llvm/lib/Support/Parallel.cpp:133
+        Lock.unlock();
+        Task();
+        SequentialQueueIsLocked = false;
----------------
andrewng wrote:
> MaskRay wrote:
> > After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.
> Yes, this can happen but I believe the key point is that it can only be a thread with a different "index". However, one subtlety is that "sequential" tasks will run on different threads and thus use a different "index".
> After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.




================
Comment at: llvm/lib/Support/Parallel.cpp:133
+        Lock.unlock();
+        Task();
+        SequentialQueueIsLocked = false;
----------------
avl wrote:
> andrewng wrote:
> > MaskRay wrote:
> > > After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.
> > Yes, this can happen but I believe the key point is that it can only be a thread with a different "index". However, one subtlety is that "sequential" tasks will run on different threads and thus use a different "index".
> > After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.
> 
> 
> After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.

After Lock.unlock(); the SequentialQueueIsLocked is true. Thus another thread could not start new sequential task until  SequentialQueueIsLocked becomes false. SequentialQueueIsLocked set to false when sequential task is finished. Thus there could not be two sequential tasks executing in parallel.


================
Comment at: llvm/lib/Support/Parallel.cpp:133
+        Lock.unlock();
+        Task();
+        SequentialQueueIsLocked = false;
----------------
avl wrote:
> avl wrote:
> > andrewng wrote:
> > > MaskRay wrote:
> > > > After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.
> > > Yes, this can happen but I believe the key point is that it can only be a thread with a different "index". However, one subtlety is that "sequential" tasks will run on different threads and thus use a different "index".
> > > After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.
> > 
> > 
> > After `Lock.unclok()`, another thread may grab the lock and run a task concurrently.
> 
> After Lock.unlock(); the SequentialQueueIsLocked is true. Thus another thread could not start new sequential task until  SequentialQueueIsLocked becomes false. SequentialQueueIsLocked set to false when sequential task is finished. Thus there could not be two sequential tasks executing in parallel.
> Yes, this can happen but I believe the key point is that it can only be a thread with a different "index". However, one subtlety is that "sequential" tasks will run on different threads and thus use a different "index".

Yes, "sequential" tasks may be run on different threads and use a different "index". I supposed that this is not a problem.


================
Comment at: llvm/unittests/Support/ParallelTest.cpp:97-121
+TEST(Parallel, TaskGroupSequential) {
+  size_t Count = 0;
+  {
+    parallel::TaskGroup tg;
+
+    tg.spawn([&]() { Count++; }, true);
+
----------------
andrewng wrote:
> I'm not entirely sure what this test case adds in terms of coverage that isn't covered by the test case that follows.
> I'm not entirely sure what this test case adds in terms of coverage that isn't covered by the test case that follows.

The idea was to test a case when previous task is already executed before new .spawn() is called.
Will remove if it seems redundant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148728/new/

https://reviews.llvm.org/D148728