[TaskScheduler] Consider the idle thread on top of the idle stack as active.

Fixing crbug.com/847501 and adding two regression tests for it.

Added SchedulerWorker::GetLastUsedTime() to support this fix and re-used
it to suppress the need for the delegate's |is_on_idle_worker_stack_|.

Tweaked WaitForWorkersCleanedUpForTesting() to allow being called
multiple times.

R=fdoray@chromium.org

Bug: 847501
Change-Id: I83f814358f679bdbee49778963ff9d05cd240adc
Reviewed-on: https://chromium-review.googlesource.com/1086978
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565288}

base/task_scheduler/scheduler_worker.cc

@@ -132,6 +132,23 @@ void SchedulerWorker::Cleanup() {
   wake_up_event_.Signal();
 }
 
+void SchedulerWorker::BeginUnusedPeriod() {
+  AutoSchedulerLock auto_lock(thread_lock_);
+  DCHECK(last_used_time_.is_null());
+  last_used_time_ = TimeTicks::Now();
+}
+
+void SchedulerWorker::EndUnusedPeriod() {
+  AutoSchedulerLock auto_lock(thread_lock_);
+  DCHECK(!last_used_time_.is_null());
+  last_used_time_ = TimeTicks();
+}
+
+TimeTicks SchedulerWorker::GetLastUsedTime() const {
+  AutoSchedulerLock auto_lock(thread_lock_);
+  return last_used_time_;
+}
+
 bool SchedulerWorker::ShouldExit() const {
   // The ordering of the checks is important below. This SchedulerWorker may be
   // released and outlive |task_tracker_| in unit tests. However, when the

base/task_scheduler/scheduler_worker.h

@@ -160,6 +160,14 @@ class BASE_EXPORT SchedulerWorker
   //   worker_ = nullptr;
   void Cleanup();
 
+  // Informs this SchedulerWorker about periods during which it is not being
+  // used. Thread-safe.
+  void BeginUnusedPeriod();
+  void EndUnusedPeriod();
+  // Returns the last time this SchedulerWorker was used. Returns a null time if
+  // this SchedulerWorker is currently in-use. Thread-safe.
+  TimeTicks GetLastUsedTime() const;
+
  private:
   friend class RefCountedThreadSafe<SchedulerWorker>;
   class Thread;
@@ -206,12 +214,16 @@ class BASE_EXPORT SchedulerWorker
   // thread is created and the second access occurs on the thread.
   scoped_refptr<SchedulerWorker> self_;
 
-  // Synchronizes access to |thread_handle_|.
+  // Synchronizes access to |thread_handle_| and |last_used_time_|.
   mutable SchedulerLock thread_lock_;
 
   // Handle for the thread managed by |this|.
   PlatformThreadHandle thread_handle_;
 
+  // The last time this worker was used by its owner (e.g. to process work or
+  // stand as a required idle thread).
+  TimeTicks last_used_time_;
+
   // Event to wake up the thread managed by |this|.
   WaitableEvent wake_up_event_{WaitableEvent::ResetPolicy::AUTOMATIC,
                                WaitableEvent::InitialState::NOT_SIGNALED};

base/task_scheduler/scheduler_worker_pool_impl.h

@@ -125,12 +125,8 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Waits until all workers are idle.
   void WaitForAllWorkersIdleForTesting();
 
-  // Waits until |n| workers have cleaned up. Tests that use this must:
-  //  - Invoke WaitForWorkersCleanedUpForTesting(n) well before any workers
-  //    have had time to clean up.
-  //  - Have a long enough |suggested_reclaim_time_| to strengthen the above.
-  //  - Only invoke this once (currently doesn't support waiting for multiple
-  //    cleanup phases in the same test).
+  // Waits until |n| workers have cleaned up (since the last call to
+  // WaitForWorkersCleanedUpForTesting() or Start() if it wasn't called yet).
   void WaitForWorkersCleanedUpForTesting(size_t n);
 
   // Returns the number of workers in this worker pool.
@@ -181,9 +177,6 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Adds |worker| to |idle_workers_stack_|.
   void AddToIdleWorkersStackLockRequired(SchedulerWorker* worker);
 
-  // Peeks from |idle_workers_stack_|.
-  const SchedulerWorker* PeekAtIdleWorkersStackLockRequired() const;
-
   // Removes |worker| from |idle_workers_stack_|.
   void RemoveFromIdleWorkersStackLockRequired(SchedulerWorker* worker);
 
@@ -305,10 +298,16 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Indicates to the delegates that workers are not permitted to cleanup.
   bool worker_cleanup_disallowed_for_testing_ = false;
 
-  // Counts the number of workers cleaned up since Start(). Tests with a custom
-  // |suggested_reclaim_time_| can wait on a specific number of workers being
-  // cleaned up via WaitForWorkersCleanedUpForTesting().
+  // Counts the number of workers cleaned up since the last call to
+  // WaitForWorkersCleanedUpForTesting() (or Start() if it wasn't called yet).
+  // |some_workers_cleaned_up_for_testing_| is true if this was ever
+  // incremented. Tests with a custom |suggested_reclaim_time_| can wait on a
+  // specific number of workers being cleaned up via
+  // WaitForWorkersCleanedUpForTesting().
   size_t num_workers_cleaned_up_for_testing_ = 0;
+#if DCHECK_IS_ON()
+  bool some_workers_cleaned_up_for_testing_ = false;
+#endif
 
   // Signaled, if non-null, when |num_workers_cleaned_up_for_testing_| is
   // incremented.

base/task_scheduler/scheduler_worker_pool_impl_unittest.cc

@@ -45,6 +45,7 @@
 #include "base/threading/thread_local_storage.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -840,6 +841,104 @@ TEST_F(TaskSchedulerWorkerPoolStandbyPolicyTest, VerifyStandbyThread) {
   EXPECT_EQ(1U, worker_pool_->NumberOfWorkersForTesting());
 }
 
+// Verify that being "the" idle thread counts as being active (i.e. won't be
+// reclaimed even if not on top of the idle stack when reclaim timeout expires).
+// Regression test for https://crbug.com/847501.
+TEST_F(TaskSchedulerWorkerPoolStandbyPolicyTest,
+       InAndOutStandbyThreadIsActive) {
+  auto sequenced_task_runner =
+      worker_pool_->CreateSequencedTaskRunnerWithTraits({});
+
+  WaitableEvent timer_started;
+
+  RepeatingTimer recurring_task;
+  sequenced_task_runner->PostTask(
+      FROM_HERE, BindLambdaForTesting([&]() {
+        recurring_task.Start(FROM_HERE, kReclaimTimeForCleanupTests / 2,
+                             DoNothing());
+        timer_started.Signal();
+      }));
+
+  timer_started.Wait();
+
+  // Running a task should have brought up a new standby thread.
+  EXPECT_EQ(2U, worker_pool_->NumberOfWorkersForTesting());
+
+  // Give extra time for a worker to cleanup : none should as the two workers
+  // are both considered "active" per the timer ticking faster than the reclaim
+  // timeout.
+  PlatformThread::Sleep(kReclaimTimeForCleanupTests * 2);
+  EXPECT_EQ(2U, worker_pool_->NumberOfWorkersForTesting());
+
+  sequenced_task_runner->PostTask(
+      FROM_HERE, BindLambdaForTesting([&]() { recurring_task.Stop(); }));
+
+  // Stopping the recurring task should let the second worker be reclaimed per
+  // not being "the" standby thread for a full reclaim timeout.
+  worker_pool_->WaitForWorkersCleanedUpForTesting(1);
+  EXPECT_EQ(1U, worker_pool_->NumberOfWorkersForTesting());
+}
+
+// Verify that being "the" idle thread counts as being active but isn't sticky.
+// Regression test for https://crbug.com/847501.
+TEST_F(TaskSchedulerWorkerPoolStandbyPolicyTest, OnlyKeepActiveStandbyThreads) {
+  auto sequenced_task_runner =
+      worker_pool_->CreateSequencedTaskRunnerWithTraits({});
+
+  // Start this test like
+  // TaskSchedulerWorkerPoolStandbyPolicyTest.InAndOutStandbyThreadIsActive and
+  // give it some time to stabilize.
+  RepeatingTimer recurring_task;
+  sequenced_task_runner->PostTask(
+      FROM_HERE, BindLambdaForTesting([&]() {
+        recurring_task.Start(FROM_HERE, kReclaimTimeForCleanupTests / 2,
+                             DoNothing());
+      }));
+
+  PlatformThread::Sleep(kReclaimTimeForCleanupTests * 2);
+  EXPECT_EQ(2U, worker_pool_->NumberOfWorkersForTesting());
+
+  // Then also flood the pool (cycling the top of the idle stack).
+  {
+    auto task_runner =
+        worker_pool_->CreateTaskRunnerWithTraits({WithBaseSyncPrimitives()});
+
+    WaitableEvent thread_running(WaitableEvent::ResetPolicy::AUTOMATIC);
+    WaitableEvent threads_continue;
+
+    RepeatingClosure thread_blocker = BindLambdaForTesting([&]() {
+      thread_running.Signal();
+      WaitWithoutBlockingObserver(&threads_continue);
+    });
+
+    for (size_t i = 0; i < kMaxTasks; ++i) {
+      task_runner->PostTask(FROM_HERE, thread_blocker);
+      thread_running.Wait();
+    }
+
+    EXPECT_EQ(kMaxTasks, worker_pool_->NumberOfWorkersForTesting());
+    threads_continue.Signal();
+
+    // Flush to ensure all references to |threads_continue| are gone before it
+    // goes out of scope.
+    task_tracker_.FlushForTesting();
+  }
+
+  // All workers should clean up but two (since the timer is still running).
+  worker_pool_->WaitForWorkersCleanedUpForTesting(kMaxTasks - 2);
+  EXPECT_EQ(2U, worker_pool_->NumberOfWorkersForTesting());
+
+  // Extra time shouldn't change this.
+  PlatformThread::Sleep(kReclaimTimeForCleanupTests * 2);
+  EXPECT_EQ(2U, worker_pool_->NumberOfWorkersForTesting());
+
+  // Stopping the timer should let the number of active threads go down to one.
+  sequenced_task_runner->PostTask(
+      FROM_HERE, BindLambdaForTesting([&]() { recurring_task.Stop(); }));
+  worker_pool_->WaitForWorkersCleanedUpForTesting(1);
+  EXPECT_EQ(1U, worker_pool_->NumberOfWorkersForTesting());
+}
+
 namespace {
 
 enum class OptionalBlockingType {

base/task_scheduler/scheduler_worker_stack.cc

@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/task_scheduler/scheduler_worker.h"
 
 namespace base {
 namespace internal {
@@ -18,6 +19,8 @@ SchedulerWorkerStack::~SchedulerWorkerStack() = default;
 
 void SchedulerWorkerStack::Push(SchedulerWorker* worker) {
   DCHECK(!Contains(worker)) << "SchedulerWorker already on stack";
+  if (!IsEmpty())
+    stack_.back()->BeginUnusedPeriod();
   stack_.push_back(worker);
 }
 
@@ -26,6 +29,8 @@ SchedulerWorker* SchedulerWorkerStack::Pop() {
     return nullptr;
   SchedulerWorker* const worker = stack_.back();
   stack_.pop_back();
+  if (!IsEmpty())
+    stack_.back()->EndUnusedPeriod();
   return worker;
 }
 
@@ -40,9 +45,12 @@ bool SchedulerWorkerStack::Contains(const SchedulerWorker* worker) const {
 }
 
 void SchedulerWorkerStack::Remove(const SchedulerWorker* worker) {
+  DCHECK(!IsEmpty());
+  DCHECK_NE(worker, stack_.back());
   auto it = std::find(stack_.begin(), stack_.end(), worker);
-  if (it != stack_.end())
-    stack_.erase(it);
+  DCHECK(it != stack_.end());
+  DCHECK_NE(TimeTicks(), (*it)->GetLastUsedTime());
+  stack_.erase(it);
 }
 
 }  // namespace internal

base/task_scheduler/scheduler_worker_stack.h

@@ -17,22 +17,26 @@ namespace internal {
 
 class SchedulerWorker;
 
-// A stack of SchedulerWorkers. Supports removal of arbitrary SchedulerWorkers.
-// DCHECKs when a SchedulerWorker is inserted multiple times. SchedulerWorkers
-// are not owned by the stack. Push() is amortized O(1). Pop(), Peek(), Size()
-// and Empty() are O(1). Contains() and Remove() are O(n).
-// This class is NOT thread-safe.
+// A stack of SchedulerWorkers which has custom logic to treat the worker on top
+// of the stack as being "in-use" (so its time in that position doesn't count
+// towards being inactive / reclaimable). Supports removal of arbitrary
+// SchedulerWorkers. DCHECKs when a SchedulerWorker is inserted multiple times.
+// SchedulerWorkers are not owned by the stack. Push() is amortized O(1). Pop(),
+// Peek(), Size() and Empty() are O(1). Contains() and Remove() are O(n). This
+// class is NOT thread-safe.
 class BASE_EXPORT SchedulerWorkerStack {
  public:
   SchedulerWorkerStack();
   ~SchedulerWorkerStack();
 
   // Inserts |worker| at the top of the stack. |worker| must not already be on
-  // the stack.
+  // the stack. Flags the SchedulerWorker previously on top of the stack, if
+  // any, as unused.
   void Push(SchedulerWorker* worker);
 
-  // Removes the top SchedulerWorker from the stack and returns it.
-  // Returns nullptr if the stack is empty.
+  // Removes the top SchedulerWorker from the stack and returns it. Returns
+  // nullptr if the stack is empty. Flags the SchedulerWorker now on top of the
+  // stack, if any, as being in-use.
   SchedulerWorker* Pop();
 
   // Returns the top SchedulerWorker from the stack, nullptr if empty.
@@ -41,7 +45,8 @@ class BASE_EXPORT SchedulerWorkerStack {
   // Returns true if |worker| is already on the stack.
   bool Contains(const SchedulerWorker* worker) const;
 
-  // Removes |worker| from the stack.
+  // Removes |worker| from the stack. Must not be invoked for the first worker
+  // on the stack.
   void Remove(const SchedulerWorker* worker);
 
   // Returns the number of SchedulerWorkers on the stack.

base/task_scheduler/scheduler_worker_stack_unittest.cc

@@ -227,19 +227,20 @@ TEST_F(TaskSchedulerWorkerStackTest, Remove) {
 TEST_F(TaskSchedulerWorkerStackTest, PushAfterRemove) {
   SchedulerWorkerStack stack;
   EXPECT_EQ(0U, stack.Size());
-  EXPECT_TRUE(stack.IsEmpty());
 
   stack.Push(worker_a_.get());
   EXPECT_EQ(1U, stack.Size());
-  EXPECT_FALSE(stack.IsEmpty());
+
+  // Need to also push worker B for this test as it's illegal to Remove() the
+  // top of the stack.
+  stack.Push(worker_b_.get());
+  EXPECT_EQ(2U, stack.Size());
 
   stack.Remove(worker_a_.get());
-  EXPECT_EQ(0U, stack.Size());
-  EXPECT_TRUE(stack.IsEmpty());
+  EXPECT_EQ(1U, stack.Size());
 
   stack.Push(worker_a_.get());
-  EXPECT_EQ(1U, stack.Size());
-  EXPECT_FALSE(stack.IsEmpty());
+  EXPECT_EQ(2U, stack.Size());
 }
 
 // Verify that Push() DCHECKs when a value is inserted twice.