BLI: use explicit task isolation, no longer part of parallel operations

After looking into task isolation issues with Sergey, we couldn't find the
reason behind the deadlocks that we are getting in T87938 and a Sprite Fright
file involving motion blur renders.

There is no apparent place where we adding or waiting on tasks in a task group
from different isolation regions, which is what is known to cause problems. Yet
it still hangs. Either we do not understand some limitation of TBB isolation,
or there is a bug in TBB, but we could not figure it out.

Instead the idea is to use isolation only where we know we need it: when
holding a mutex lock and then doing some multithreaded operation within that
locked region. Three places where we do this now:
* Generated images
* Cached BVH tree building
* OpenVDB lazy grid loading

Compared to the more automatic approach previously used, there is the downside
that it is easy to miss places where we need isolation. Yet doing it more
automatically is also causing unexpected issue and bugs that we found no
solution for, so this seems better.

Patch implemented by Sergey and me.

Differential Revision: https://developer.blender.org/D11603

1 files changed, 9 insertions, 54 deletions

diff --git a/source/blender/blenlib/intern/task_pool.cc b/source/blender/blenlib/intern/task_pool.cc
index d72674c1c00..6250c1b9986 100644
--- a/source/blender/blenlib/intern/task_pool.cc
+++ b/source/blender/blenlib/intern/task_pool.cc
@@ -22,7 +22,6 @@
 
 #include <cstdlib>
 #include <memory>
-#include <thread>
 #include <utility>
 
 #include "MEM_guardedalloc.h"
@@ -156,7 +155,6 @@ enum TaskPoolType {
 struct TaskPool {
   TaskPoolType type;
   bool use_threads;
-  TaskIsolation task_isolation;
 
   ThreadMutex user_mutex;
   void *userdata;
@@ -164,8 +162,6 @@ struct TaskPool {
 #ifdef WITH_TBB
   /* TBB task pool. */
   TBBTaskGroup tbb_group;
-  /* This is used to detect a common way to accidentally create a deadlock with task isolation. */
-  std::thread::id task_pool_create_thread_id;
 #endif
   volatile bool is_suspended;
   BLI_mempool *suspended_mempool;
@@ -179,33 +175,9 @@ struct TaskPool {
 /* Execute task. */
 void Task::operator()() const
 {
-#ifdef WITH_TBB
-  if (pool->task_isolation == TASK_ISOLATION_ON) {
-    tbb::this_task_arena::isolate([this] { run(pool, taskdata); });
-    return;
-  }
-#endif
   run(pool, taskdata);
 }
 
-static void assert_on_valid_thread(TaskPool *pool)
-{
-  /* TODO: Remove this `return` to enable the check. */
-  return;
-#ifdef DEBUG
-#  ifdef WITH_TBB
-  if (pool->task_isolation == TASK_ISOLATION_ON) {
-    const std::thread::id current_id = std::this_thread::get_id();
-    /* This task pool is modified from different threads. To avoid deadlocks, `TASK_ISOLATION_OFF`
-     * has to be used. Task isolation can still be used in a more fine-grained way within the
-     * tasks, but should not be enabled for the entire task pool. */
-    BLI_assert(pool->task_pool_create_thread_id == current_id);
-  }
-#  endif
-#endif
-  UNUSED_VARS_NDEBUG(pool);
-}
-
 /* TBB Task Pool.
  *
  * Task pool using the TBB scheduler for tasks. When building without TBB
@@ -391,10 +363,7 @@ static void background_task_pool_free(TaskPool *pool)
 
 /* Task Pool */
 
-static TaskPool *task_pool_create_ex(void *userdata,
-                                     TaskPoolType type,
-                                     TaskPriority priority,
-                                     TaskIsolation task_isolation)
+static TaskPool *task_pool_create_ex(void *userdata, TaskPoolType type, TaskPriority priority)
 {
   const bool use_threads = BLI_task_scheduler_num_threads() > 1 && type != TASK_POOL_NO_THREADS;
 
@@ -410,11 +379,6 @@ static TaskPool *task_pool_create_ex(void *userdata,
 
   pool->type = type;
   pool->use_threads = use_threads;
-  pool->task_isolation = task_isolation;
-
-#ifdef WITH_TBB
-  pool->task_pool_create_thread_id = std::this_thread::get_id();
-#endif
 
   pool->userdata = userdata;
   BLI_mutex_init(&pool->user_mutex);
@@ -437,9 +401,9 @@ static TaskPool *task_pool_create_ex(void *userdata,
 /**
  * Create a normal task pool. Tasks will be executed as soon as they are added.
  */
-TaskPool *BLI_task_pool_create(void *userdata, TaskPriority priority, TaskIsolation task_isolation)
+TaskPool *BLI_task_pool_create(void *userdata, TaskPriority priority)
 {
-  return task_pool_create_ex(userdata, TASK_POOL_TBB, priority, task_isolation);
+  return task_pool_create_ex(userdata, TASK_POOL_TBB, priority);
 }
 
 /**
@@ -454,11 +418,9 @@ TaskPool *BLI_task_pool_create(void *userdata, TaskPriority priority, TaskIsolat
  * they could end never being executed, since the 'fallback' background thread is already
  * busy with parent task in single-threaded context).
  */
-TaskPool *BLI_task_pool_create_background(void *userdata,
-                                          TaskPriority priority,
-                                          TaskIsolation task_isolation)
+TaskPool *BLI_task_pool_create_background(void *userdata, TaskPriority priority)
 {
-  return task_pool_create_ex(userdata, TASK_POOL_BACKGROUND, priority, task_isolation);
+  return task_pool_create_ex(userdata, TASK_POOL_BACKGROUND, priority);
 }
 
 /**
@@ -466,11 +428,9 @@ TaskPool *BLI_task_pool_create_background(void *userdata,
  * for until BLI_task_pool_work_and_wait() is called. This helps reducing threading
  * overhead when pushing huge amount of small initial tasks from the main thread.
  */
-TaskPool *BLI_task_pool_create_suspended(void *userdata,
-                                         TaskPriority priority,
-                                         TaskIsolation task_isolation)
+TaskPool *BLI_task_pool_create_suspended(void *userdata, TaskPriority priority)
 {
-  return task_pool_create_ex(userdata, TASK_POOL_TBB_SUSPENDED, priority, task_isolation);
+  return task_pool_create_ex(userdata, TASK_POOL_TBB_SUSPENDED, priority);
 }
 
 /**
@@ -479,8 +439,7 @@ TaskPool *BLI_task_pool_create_suspended(void *userdata,
  */
 TaskPool *BLI_task_pool_create_no_threads(void *userdata)
 {
-  return task_pool_create_ex(
-      userdata, TASK_POOL_NO_THREADS, TASK_PRIORITY_HIGH, TASK_ISOLATION_ON);
+  return task_pool_create_ex(userdata, TASK_POOL_NO_THREADS, TASK_PRIORITY_HIGH);
 }
 
 /**
@@ -489,7 +448,7 @@ TaskPool *BLI_task_pool_create_no_threads(void *userdata)
  */
 TaskPool *BLI_task_pool_create_background_serial(void *userdata, TaskPriority priority)
 {
-  return task_pool_create_ex(userdata, TASK_POOL_BACKGROUND_SERIAL, priority, TASK_ISOLATION_ON);
+  return task_pool_create_ex(userdata, TASK_POOL_BACKGROUND_SERIAL, priority);
 }
 
 void BLI_task_pool_free(TaskPool *pool)
@@ -517,8 +476,6 @@ void BLI_task_pool_push(TaskPool *pool,
                         bool free_taskdata,
                         TaskFreeFunction freedata)
 {
-  assert_on_valid_thread(pool);
-
   Task task(pool, run, taskdata, free_taskdata, freedata);
 
   switch (pool->type) {
@@ -536,8 +493,6 @@ void BLI_task_pool_push(TaskPool *pool,
 
 void BLI_task_pool_work_and_wait(TaskPool *pool)
 {
-  assert_on_valid_thread(pool);
-
   switch (pool->type) {
     case TASK_POOL_TBB:
     case TASK_POOL_TBB_SUSPENDED: