I was curious about what was crashing here, and found I couldn't reproduce it. Turns out that my build with Clang seems works fine, but GCC it's crashing.

For some strange reason it's failing to copy task parameters for suspended tasks. I tried storing the task parameter a plain struct without any methods, since I suspected placement new or move constructors could be problematic. But the same issue happens. Somehow the presence of the TBB task run code in the same function causes this to fail.

At this point I'm suspecting a compiler bug, though those are of course rare. This works for me, but I don't know why exactly.

--- a/source/blender/blenlib/intern/task_pool.cc
+++ b/source/blender/blenlib/intern/task_pool.cc
@@ -180,6 +180,13 @@ static void tbb_task_pool_run(TaskPool *pool, Task &&task)
     /* Suspended task that will be executed in work_and_wait(). */
     Task *task_mem = (Task *)BLI_mempool_alloc(pool->suspended_mempool);
     new (task_mem) Task(std::move(task));
+#ifdef __GNUC__
+    /* Work around apparent compiler bug where task is not properly copied
+     * to task_mem. This appears unrelated to the use of placement new or
+     * move semantics, happens even writing to a plain C struct. Rather the
+     * call into TBB seems to have some indirect effect. */
+    std::atomic_thread_fence(std::memory_order_release);
+#endif
   }
 #ifdef WITH_TBB
   else if (pool->use_threads) {

Suspended pools were introduced to reduce huge threading overhead on machines with 16+ threads. On such configurations it was faster to avoid any threading overhead on every task push.
With TBB we should eliminate suspended pools, as one would expect TBB will have less overhead.

The idea of task_parallel_range_pool was to allow iteration over multiple ranges with maximum CPU utilization. Imagine you have 2 ranges of 24 elements to be covered on 16 threads machine. In sequential call of task_parallel_range you would have utilization like this:

• All 16 threads are busy handling first 16 tasks of the first range
• 8 threads are busy handling 8 remaining elements of the first range
• All 16 threads are busy handling first 16 tasks of the second range
• 8 threads are busy handling 8 remaining elements of the second range

If there is no dependency in handling the ranges you can imagine that more efficient would be to:

• All 16 threads are busy handling first 16 tasks of the first range.
• 8 threads are busy handling 8 remaining elements of first range, 8 threads are busy handling 8 first elements of the second range.
• All 16 threads are busy handling remaining 16 elements of the second range.

The intent was to sue this for BKE_subdiv_foreach_subdiv_geometry (see TODO and NOTE in there).

For the performance use something like Vincent's rig. That indicates threading overhead in depsgraph the best. It's also something where more CPU cores you have more threading overhead you have with the old code. While on 4 core 8 threads SKL i7 it wasn't bad, on 22 core 44 threads BDW Xeon the threading overhead is very noticeable in the profiler.

Looking at the TBB documentation we could map task_parallel_range_pool to a custom Iteration Space (https://software.intel.com/en-us/node/506065). I will do some experiments.

@Jeroen Bakker (jbakker), i wouldn't spend too much time on this. It would be possible to implement one way or another. And since currently this codepath isn't in use and requires some changes in the code where it was intended to be used, can happen as part of Modifier/Subdiv optimization.

The patch is still stating it's WIP, withotu reviewers assigned. Will you give an explicit poke when you will consider patch ready for review?

In D7475#179672, @Sergey Sharybin (sergey) wrote:

Suspended pools were introduced to reduce huge threading overhead on machines with 16+ threads. On such configurations it was faster to avoid any threading overhead on every task push.

Is this about suspended pools or the local queue (which is also being removed in this patch)? I'm not sure how suspended pools would improve performance.

@Brecht Van Lommel (brecht), Actually, local queue is not used during suspension (arguably, it should from performance point of view, but then pool isn't really suspended). The actual speedup was coming from the change of code flow from

BLI_mutex_lock(...)
BLI_addtail(...)
BLI_condition_notify_one(...)
BLI_mutex_unlock(...)

// ...

BLI_mutex_lock(...)
BLI_addtail(...)
BLI_condition_notify_one(...)
BLI_mutex_unlock(...)

// ... as many times as you've got new tasks to be pushed ...

to

// Single lock block, but notify all.
BLI_mutex_lock(...)
BLI_movelisttolist(...)
BLI_condition_notify_all(...)
BLI_mutex_unlock(...)

Testing on AMD Ryzen Threadripper 2990WX 32-Core.

Agent 327 , 10_03_B.anim.blend. Animation playback goes from 9.5-10.5 FPS to 13.0-14.0 FPS, which is pretty significant.

I imagine that is where we'd see the most speedup, many cores and multiple animated characters.

In D7475#182056, @Sergey Sharybin (sergey) wrote:

Tested on Linux Xeon E5-2699 v4 and got FPS boost from 12 to 17 using Spring's 04_010_A.anim.blend. And that's not that many CPU cores compared to Brecht. That's quite awesome speedup to see :)

I've also tested with simple scenes on my macOS laptop. Seemed all nice and stable as well.

Still need to test Windows. Maybe this is something where @Ray Molenkamp (LazyDodo) can help? WINK WINK

I don't have the spring files, so no idea on the perf, however all unit tests seem to pass.