[ThreadPool]: Best effort max tasks is increased only after a delay.

Best effort max tasks always acts like MayBlock.
This is to prevent issue where many WILL_BLOCK BEST_EFFORT tasks increase the
thread pool capacity, which is undesirable.

This new behavior is similar to FixedMaxBestEffortTasks, but keeps the increase
around to prevent deadlocks.
https://uma.googleplex.com/p/chrome/variations/?sid=4a990d53992445c75cf9665227ab221c

Bug: 1026785
Change-Id: I4be2f478c730cc75f8d358665edec72ad651902f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300343
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796545}

base/task/task_features.cc

@@ -17,9 +17,6 @@ const Feature kNoDetachBelowInitialCapacity = {
 const Feature kMayBlockWithoutDelay = {"MayBlockWithoutDelay",
                                        base::FEATURE_DISABLED_BY_DEFAULT};
 
-const Feature kFixedMaxBestEffortTasks = {"FixedMaxBestEffortTasks",
-                                          base::FEATURE_DISABLED_BY_DEFAULT};
-
 #if defined(OS_WIN) || defined(OS_APPLE)
 const Feature kUseNativeThreadPool = {"UseNativeThreadPool",
                                       base::FEATURE_DISABLED_BY_DEFAULT};

base/task/task_features.h

@@ -23,12 +23,6 @@ extern const BASE_EXPORT Feature kNoDetachBelowInitialCapacity;
 // instead of waiting for a threshold in the foreground thread group.
 extern const BASE_EXPORT Feature kMayBlockWithoutDelay;
 
-// Under this feature, best effort capacity is never increased.
-// While it's unlikely we'd ship this as-is, this experiment allows us to
-// determine whether blocked worker replacement logic on best-effort tasks has
-// any impact on guardian metrics.
-extern const BASE_EXPORT Feature kFixedMaxBestEffortTasks;
-
 #if defined(OS_WIN) || defined(OS_APPLE)
 #define HAS_NATIVE_THREAD_POOL() 1
 #else

base/task/thread_pool/thread_group_impl.h

@@ -122,8 +122,9 @@ class BASE_EXPORT ThreadGroupImpl : public ThreadGroup {
   // Returns the number of workers in this thread group.
   size_t NumberOfWorkersForTesting() const;
 
-  // Returns |max_tasks_|.
+  // Returns |max_tasks_|/|max_best_effort_tasks_|.
   size_t GetMaxTasksForTesting() const;
+  size_t GetMaxBestEffortTasksForTesting() const;
 
   // Returns the number of workers that are idle (i.e. not running tasks).
   size_t NumberOfIdleWorkersForTesting() const;
@@ -138,6 +139,8 @@ class BASE_EXPORT ThreadGroupImpl : public ThreadGroup {
   friend class ThreadGroupImplMayBlockTest;
   FRIEND_TEST_ALL_PREFIXES(ThreadGroupImplBlockingTest,
                            ThreadBlockUnblockPremature);
+  FRIEND_TEST_ALL_PREFIXES(ThreadGroupImplBlockingTest,
+                           ThreadBlockUnblockPrematureBestEffort);
 
   // ThreadGroup:
   void UpdateSortKey(TaskSource::Transaction transaction) override;
@@ -211,12 +214,13 @@ class BASE_EXPORT ThreadGroupImpl : public ThreadGroup {
   void IncrementTasksRunningLockRequired(TaskPriority priority)
       EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
-  // Increments/decrements the number of tasks that can run in this thread
-  // group.  May only be called in a scope where a task is running with
-  // |priority|.
-  void DecrementMaxTasksLockRequired(TaskPriority priority)
+  // Increments/decrements the number of [best effort] tasks that can run in
+  // this thread group.
+  void DecrementMaxTasksLockRequired() EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  void IncrementMaxTasksLockRequired() EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  void DecrementMaxBestEffortTasksLockRequired()
       EXCLUSIVE_LOCKS_REQUIRED(lock_);
-  void IncrementMaxTasksLockRequired(TaskPriority priority)
+  void IncrementMaxBestEffortTasksLockRequired()
       EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   // Values set at Start() and never modified afterwards.
@@ -244,7 +248,6 @@ class BASE_EXPORT ThreadGroupImpl : public ThreadGroup {
     WorkerThreadObserver* worker_thread_observer = nullptr;
 
     bool may_block_without_delay;
-    bool fixed_max_best_effort_tasks;
 
     // Threshold after which the max tasks is increased to compensate for a
     // worker that is within a MAY_BLOCK ScopedBlockingCall.

base/task/thread_pool/thread_group_impl_unittest.cc

@@ -916,15 +916,20 @@ class ThreadGroupImplBlockingTest
   // Saturates the thread group with a task that first blocks, waits to be
   // unblocked, then exits.
   void SaturateWithBlockingTasks(
-      const NestedBlockingType& nested_blocking_type) {
+      const NestedBlockingType& nested_blocking_type,
+      TaskPriority priority = TaskPriority::USER_BLOCKING) {
     TestWaitableEvent threads_running;
 
+    const scoped_refptr<TaskRunner> task_runner = test::CreatePooledTaskRunner(
+        {MayBlock(), WithBaseSyncPrimitives(), priority},
+        &mock_pooled_task_runner_delegate_);
+
     RepeatingClosure threads_running_barrier = BarrierClosure(
         kMaxTasks,
         BindOnce(&TestWaitableEvent::Signal, Unretained(&threads_running)));
 
     for (size_t i = 0; i < kMaxTasks; ++i) {
-      task_runner_->PostTask(
+      task_runner->PostTask(
           FROM_HERE, BindLambdaForTesting([this, &threads_running_barrier,
                                            nested_blocking_type]() {
             NestedScopedBlockingCall nested_scoped_blocking_call(
@@ -938,15 +943,20 @@ class ThreadGroupImplBlockingTest
 
   // Saturates the thread group with a task that waits for other tasks without
   // entering a ScopedBlockingCall, then exits.
-  void SaturateWithBusyTasks() {
+  void SaturateWithBusyTasks(
+      TaskPriority priority = TaskPriority::USER_BLOCKING) {
     TestWaitableEvent threads_running;
 
+    const scoped_refptr<TaskRunner> task_runner = test::CreatePooledTaskRunner(
+        {MayBlock(), WithBaseSyncPrimitives(), priority},
+        &mock_pooled_task_runner_delegate_);
+
     RepeatingClosure threads_running_barrier = BarrierClosure(
         kMaxTasks,
         BindOnce(&TestWaitableEvent::Signal, Unretained(&threads_running)));
     // Posting these tasks should cause new workers to be created.
     for (size_t i = 0; i < kMaxTasks; ++i) {
-      task_runner_->PostTask(
+      task_runner->PostTask(
           FROM_HERE, BindLambdaForTesting([this, &threads_running_barrier]() {
             threads_running_barrier.Run();
             busy_threads_continue_.Wait();
@@ -1014,6 +1024,30 @@ TEST_P(ThreadGroupImplBlockingTest, ThreadBlockedUnblocked) {
   EXPECT_EQ(thread_group_->GetMaxTasksForTesting(), kMaxTasks);
 }
 
+// Verify that SaturateWithBlockingTasks() of BEST_EFFORT tasks causes max best
+// effort tasks to increase and creates a worker if needed. Also verify that
+// UnblockBlockingTasks() decreases max best effort tasks after an increase.
+TEST_P(ThreadGroupImplBlockingTest, ThreadBlockedUnblockedBestEffort) {
+  CreateAndStartThreadGroup();
+
+  ASSERT_EQ(thread_group_->GetMaxTasksForTesting(), kMaxTasks);
+  ASSERT_EQ(thread_group_->GetMaxBestEffortTasksForTesting(), kMaxTasks);
+
+  SaturateWithBlockingTasks(GetParam(), TaskPriority::BEST_EFFORT);
+
+  // Forces |kMaxTasks| extra workers to be instantiated by posting tasks. This
+  // should not block forever.
+  SaturateWithBusyTasks(TaskPriority::BEST_EFFORT);
+
+  EXPECT_EQ(thread_group_->NumberOfWorkersForTesting(), 2 * kMaxTasks);
+
+  UnblockBusyTasks();
+  UnblockBlockingTasks();
+  task_tracker_.FlushForTesting();
+  EXPECT_EQ(thread_group_->GetMaxTasksForTesting(), kMaxTasks);
+  EXPECT_EQ(thread_group_->GetMaxBestEffortTasksForTesting(), kMaxTasks);
+}
+
 // Verify that flooding the thread group with more BEST_EFFORT tasks than
 // kMaxBestEffortTasks doesn't prevent USER_VISIBLE tasks from running.
 TEST_P(ThreadGroupImplBlockingTest, TooManyBestEffortTasks) {
@@ -1286,6 +1320,37 @@ TEST_F(ThreadGroupImplBlockingTest, ThreadBlockUnblockPremature) {
   EXPECT_EQ(thread_group_->GetMaxTasksForTesting(), kMaxTasks);
 }
 
+// Verify that if a BEST_EFFORT task enters the scope of a WILL_BLOCK
+// ScopedBlockingCall, but exits the scope before the MayBlock threshold is
+// reached, that the max best effort tasks does not increase.
+TEST_F(ThreadGroupImplBlockingTest, ThreadBlockUnblockPrematureBestEffort) {
+  // Create a thread group with an infinite MayBlock threshold so that a
+  // MAY_BLOCK ScopedBlockingCall never increases the max tasks.
+  CreateAndStartThreadGroup(TimeDelta::Max(),  // |suggested_reclaim_time|
+                            kMaxTasks,         // |max_tasks|
+                            kMaxTasks,         // |max_best_effort_tasks|
+                            nullptr,           // |worker_observer|
+                            TimeDelta::Max()   // |may_block_threshold|
+  );
+  ASSERT_EQ(thread_group_->GetMaxTasksForTesting(), kMaxTasks);
+  ASSERT_EQ(thread_group_->GetMaxBestEffortTasksForTesting(), kMaxTasks);
+
+  SaturateWithBlockingTasks(NestedBlockingType(BlockingType::WILL_BLOCK,
+                                               OptionalBlockingType::NO_BLOCK,
+                                               BlockingType::WILL_BLOCK),
+                            TaskPriority::BEST_EFFORT);
+  PlatformThread::Sleep(
+      2 * thread_group_->blocked_workers_poll_period_for_testing());
+  EXPECT_GE(thread_group_->NumberOfWorkersForTesting(), kMaxTasks);
+  EXPECT_EQ(thread_group_->GetMaxTasksForTesting(), 2 * kMaxTasks);
+  EXPECT_EQ(thread_group_->GetMaxBestEffortTasksForTesting(), kMaxTasks);
+
+  UnblockBlockingTasks();
+  task_tracker_.FlushForTesting();
+  EXPECT_EQ(thread_group_->GetMaxTasksForTesting(), kMaxTasks);
+  EXPECT_EQ(thread_group_->GetMaxBestEffortTasksForTesting(), kMaxTasks);
+}
+
 // Verify that if max tasks is incremented because of a MAY_BLOCK
 // ScopedBlockingCall, it isn't incremented again when there is a nested
 // WILL_BLOCK ScopedBlockingCall.

testing/variations/fieldtrial_testing_config.json

@@ -2914,25 +2914,6 @@
             ]
         }
     ],
-    "FixedMaxBestEffortTasks": [
-        {
-            "platforms": [
-                "android",
-                "chromeos",
-                "linux",
-                "mac",
-                "windows"
-            ],
-            "experiments": [
-                {
-                    "name": "Enabled",
-                    "enable_features": [
-                        "FixedMaxBestEffortTasks"
-                    ]
-                }
-            ]
-        }
-    ],
     "FlexNG": [
         {
             "platforms": [