Add TaskScheduler.NumTasksBetweenWaits.[pool] histogram.

base/task_scheduler/scheduler_worker_pool_impl_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/task_scheduler/scheduler_worker_pool_impl.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <unordered_set>
 #include <vector>
 
 #include "base/atomicops.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/metrics/histogram.h"
+#include "base/metrics/histogram_samples.h"
+#include "base/metrics/statistics_recorder.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task_runner.h"
 #include "base/task_scheduler/delayed_task_manager.h"
 #include "base/task_scheduler/scheduler_worker_pool_params.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/task_scheduler/sequence_sort_key.h"
 #include "base/task_scheduler/task_tracker.h"
 #include "base/task_scheduler/test_task_factory.h"
 #include "base/test/gtest_util.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/simple_thread.h"
 #include "base/threading/thread_checker_impl.h"
 #include "base/threading/thread_local_storage.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
 namespace internal {
 namespace {
 
-const size_t kNumWorkersInWorkerPool = 4;
-const size_t kNumThreadsPostingTasks = 4;
-const size_t kNumTasksPostedPerThread = 150;
+constexpr size_t kNumWorkersInWorkerPool = 4;
+constexpr size_t kNumThreadsPostingTasks = 4;
+constexpr size_t kNumTasksPostedPerThread = 150;
+// This can't be lower because Windows' WaitableEvent wakes up too early when a
+// small timeout is used. This results in many spurious wake ups before a worker
+// is allowed to detach.
 constexpr TimeDelta kReclaimTimeForDetachTests =
-    TimeDelta::FromMilliseconds(10);
+    TimeDelta::FromMilliseconds(200);
+constexpr TimeDelta kExtraTimeToWaitForDetach =
+    TimeDelta::FromMilliseconds(200);
 
 using IORestriction = SchedulerWorkerPoolParams::IORestriction;
 
 class TestDelayedTaskManager : public DelayedTaskManager {
  public:
   TestDelayedTaskManager() : DelayedTaskManager(Bind(&DoNothing)) {}
 
   void SetCurrentTime(TimeTicks now) { now_ = now; }
 
   // DelayedTaskManager:
   TimeTicks Now() const override { return now_; }
 
  private:
   TimeTicks now_ = TimeTicks::Now();
 
   DISALLOW_COPY_AND_ASSIGN(TestDelayedTaskManager);
 };
 
 class TaskSchedulerWorkerPoolImplTest
     : public testing::TestWithParam<ExecutionMode> {
  protected:
   TaskSchedulerWorkerPoolImplTest() = default;
 
   void SetUp() override {
-    InitializeWorkerPool(TimeDelta::Max());
+    InitializeWorkerPool(TimeDelta::Max(), kNumWorkersInWorkerPool);
   }
 
   void TearDown() override {
     worker_pool_->WaitForAllWorkersIdleForTesting();
     worker_pool_->JoinForTesting();
   }
 
-  void InitializeWorkerPool(const TimeDelta& suggested_reclaim_time) {
+  void InitializeWorkerPool(const TimeDelta& suggested_reclaim_time,
+                            size_t num_workers) {
     worker_pool_ = SchedulerWorkerPoolImpl::Create(
-        SchedulerWorkerPoolParams("TestWorkerPoolWithFileIO",
-                                  ThreadPriority::NORMAL,
-                                  IORestriction::ALLOWED,
-                                  kNumWorkersInWorkerPool,
+        SchedulerWorkerPoolParams("TestWorkerPool", ThreadPriority::NORMAL,
+                                  IORestriction::ALLOWED, num_workers,
                                   suggested_reclaim_time),
         Bind(&TaskSchedulerWorkerPoolImplTest::ReEnqueueSequenceCallback,
              Unretained(this)),
         &task_tracker_, &delayed_task_manager_);
     ASSERT_TRUE(worker_pool_);
   }
 
   std::unique_ptr<SchedulerWorkerPoolImpl> worker_pool_;
 
   TaskTracker task_tracker_;
@@ skipping 334 matching lines @@
   void InitializeThreadChecker() {
     thread_checker_.reset(new ThreadCheckerImpl());
   }
 
   void CheckValidThread() {
     EXPECT_TRUE(thread_checker_->CalledOnValidThread());
   }
 
  protected:
   void SetUp() override {
-    InitializeWorkerPool(kReclaimTimeForDetachTests);
+    InitializeWorkerPool(kReclaimTimeForDetachTests, kNumWorkersInWorkerPool);
   }
 
   TaskSchedulerWorkerPoolSingleThreadedTest() = default;
 
  private:
   std::unique_ptr<ThreadCheckerImpl> thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerPoolSingleThreadedTest);
 };
 
@@ skipping 11 matching lines @@
       Bind(&TaskSchedulerWorkerPoolSingleThreadedTest::InitializeThreadChecker,
            Unretained(this)));
   WaitableEvent task_waiter(WaitableEvent::ResetPolicy::AUTOMATIC,
                             WaitableEvent::InitialState::NOT_SIGNALED);
   single_thread_task_runner->PostTask(
       FROM_HERE, Bind(&WaitableEvent::Signal, Unretained(&task_waiter)));
   task_waiter.Wait();
   worker_pool_->WaitForAllWorkersIdleForTesting();
 
   // Give the worker pool a chance to reclaim its threads.
-  PlatformThread::Sleep(
-      kReclaimTimeForDetachTests + TimeDelta::FromMilliseconds(200));
+  PlatformThread::Sleep(kReclaimTimeForDetachTests + kExtraTimeToWaitForDetach);
 
   worker_pool_->DisallowWorkerDetachmentForTesting();
 
   single_thread_task_runner->PostTask(
       FROM_HERE,
       Bind(&TaskSchedulerWorkerPoolSingleThreadedTest::CheckValidThread,
            Unretained(this)));
   single_thread_task_runner->PostTask(
       FROM_HERE, Bind(&WaitableEvent::Signal, Unretained(&task_waiter)));
   task_waiter.Wait();
@@ skipping 18 matching lines @@
     count_waiter->Signal();
     waiter_.Wait();
   }
 
  protected:
   TaskSchedulerWorkerPoolCheckTlsReuse() :
       waiter_(WaitableEvent::ResetPolicy::MANUAL,
               WaitableEvent::InitialState::NOT_SIGNALED) {}
 
   void SetUp() override {
-    InitializeWorkerPool(kReclaimTimeForDetachTests);
+    InitializeWorkerPool(kReclaimTimeForDetachTests, kNumWorkersInWorkerPool);
   }
 
   subtle::Atomic32 zero_tls_values_ = 0;
 
   WaitableEvent waiter_;
 
  private:
   ThreadLocalStorage::Slot slot_;
 
   DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerPoolCheckTlsReuse);
@@ skipping 18 matching lines @@
   }
 
   // Release tasks waiting on |waiter_|.
   waiter_.Signal();
   worker_pool_->WaitForAllWorkersIdleForTesting();
 
   // All threads should be done running by now, so reset for the next phase.
   waiter_.Reset();
 
   // Give the worker pool a chance to detach its threads.
-  PlatformThread::Sleep(
-      kReclaimTimeForDetachTests + TimeDelta::FromMilliseconds(200));
+  PlatformThread::Sleep(kReclaimTimeForDetachTests + kExtraTimeToWaitForDetach);
 
   worker_pool_->DisallowWorkerDetachmentForTesting();
 
   // Saturate and count the threads that do not have the magic TLS value. If the
   // value is not there, that means we're at a new thread.
   std::vector<std::unique_ptr<WaitableEvent>> count_waiters;
   for (auto& factory : factories) {
     count_waiters.push_back(WrapUnique(new WaitableEvent(
         WaitableEvent::ResetPolicy::MANUAL,
         WaitableEvent::InitialState::NOT_SIGNALED)));
     ASSERT_TRUE(factory->PostTask(
           PostNestedTask::NO,
           Bind(&TaskSchedulerWorkerPoolCheckTlsReuse::CountZeroTlsValuesAndWait,
                Unretained(this),
                count_waiters.back().get())));
     factory->WaitForAllTasksToRun();
   }
 
   // Wait for all counters to complete.
   for (auto& count_waiter : count_waiters)
     count_waiter->Wait();
 
   EXPECT_GT(subtle::NoBarrier_Load(&zero_tls_values_), 0);
 
   // Release tasks waiting on |waiter_|.
   waiter_.Signal();
 }
 
+namespace {
+
+class TaskSchedulerWorkerPoolHistogramTest
+    : public TaskSchedulerWorkerPoolImplTest {
+ public:
+  TaskSchedulerWorkerPoolHistogramTest() = default;
+
+ protected:
+  void SetUp() override {}
+
+  void TearDown() override { worker_pool_->JoinForTesting(); }
+
+ private:
+  std::unique_ptr<StatisticsRecorder> statistics_recorder_ =
+      StatisticsRecorder::CreateTemporaryForTesting();
+
+ protected:
+  HistogramBase* const num_tasks_between_waits_histogram_ =
+      Histogram::FactoryGet(

[robliao, 2016/09/23 19:44:19]

Abstract this as a static call on the SchedulerWorkerPoolImpl.

[fdoray, 2016/09/26 12:22:05]

FRIEND_TEST_ALL_PREFIXES in SchedulerWorkerPoolImpl instead? That way I don't
have to add a static method that wouldn't have existed otherwise.

[robliao, 2016/09/26 17:19:01]

I'd say go for the static method. It's relatively cheap (if not free via
optimizations), and pretty clean. This way the test doesn't have access to all
the internals for SchedulerWorkerPoolImpl.

[fdoray, 2016/09/26 19:14:36]

WDYT of a public accessor for |num_tasks_between_waits_histogram_|? If I add a
static method, I'm afraid that it won't be clear that the cached
|num_tasks_between_waits_histogram_| must be used instead of calling the static
method every time that we record to the histogram.

[robliao, 2016/09/26 19:26:38]

A public accessor sounds fine too. Accomplishes a similar result.

[fdoray, 2016/09/26 19:32:38]

Done.

+          "TaskScheduler.NumTasksBetweenWaits.TestWorkerPoolPool",
+          1,
+          100,
+          50,
+          HistogramBase::kUmaTargetedHistogramFlag);
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerPoolHistogramTest);
+};
+
+}  // namespace
+
+static void IncrementCountAndWaitForEvent() {}

[robliao, 2016/09/23 19:44:19]

Stray function?

[fdoray, 2016/09/26 12:22:05]

Done.

+
+TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBetweenWaits) {
+  WaitableEvent event(WaitableEvent::ResetPolicy::MANUAL,
+                      WaitableEvent::InitialState::NOT_SIGNALED);
+  InitializeWorkerPool(TimeDelta::Max(), kNumWorkersInWorkerPool);
+  auto task_runner = worker_pool_->CreateTaskRunnerWithTraits(
+      TaskTraits(), ExecutionMode::SEQUENCED);
+
+  // Post a task.
+  task_runner->PostTask(FROM_HERE,
+                        Bind(&WaitableEvent::Wait, Unretained(&event)));
+
+  // Post 2 more tasks while the first task hasn't completed its execution. It
+  // is guaranteed that these tasks will run immediately after the first task,
+  // without allowing the worker to sleep.
+  task_runner->PostTask(FROM_HERE, Bind(&DoNothing));
+  task_runner->PostTask(FROM_HERE, Bind(&DoNothing));
+
+  // Allow tasks to run and wait until the SchedulerWorker is idle.
+  event.Signal();
+  worker_pool_->WaitForAllWorkersIdleForTesting();
+
+  // Wake up the SchedulerWorker that just became idle by posting a task and
+  // wait until it becomes idle again. The SchedulerWorker should record the
+  // TaskScheduler.NumTasksBetweenWaits.* histogram on wake up.
+  task_runner->PostTask(FROM_HERE, Bind(&DoNothing));
+  worker_pool_->WaitForAllWorkersIdleForTesting();
+
+  // Verify that counts were recorded to the histogram as expected.
+  EXPECT_EQ(0,
+            num_tasks_between_waits_histogram_->SnapshotDelta()->GetCount(0));
+  EXPECT_EQ(1,
+            num_tasks_between_waits_histogram_->SnapshotDelta()->GetCount(3));
+  EXPECT_EQ(0,
+            num_tasks_between_waits_histogram_->SnapshotDelta()->GetCount(10));
+}
+
+static void SignalAndWaitEvent(WaitableEvent* signal_event,

[robliao, 2016/09/23 19:44:19]

Wrap in anonymous namespace, remove static.

[fdoray, 2016/09/26 12:22:05]

Done.

+                               WaitableEvent* wait_event) {
+  signal_event->Signal();
+  wait_event->Wait();
+}
+
+TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBetweenWaitsWithDetach) {
+  WaitableEvent tasks_can_exit_event(WaitableEvent::ResetPolicy::MANUAL,
+                                     WaitableEvent::InitialState::NOT_SIGNALED);
+  InitializeWorkerPool(kReclaimTimeForDetachTests, kNumWorkersInWorkerPool);
+  auto task_runner = worker_pool_->CreateTaskRunnerWithTraits(
+      TaskTraits(), ExecutionMode::PARALLEL);
+
+  // Post tasks to saturate the pool.
+  std::vector<std::unique_ptr<WaitableEvent>> task_started_events;
+  for (int i = 0; i < kNumWorkersInWorkerPool; ++i) {
+    task_started_events.push_back(
+        MakeUnique<WaitableEvent>(WaitableEvent::ResetPolicy::MANUAL,
+                                  WaitableEvent::InitialState::NOT_SIGNALED));
+    task_runner->PostTask(
+        FROM_HERE,
+        Bind(&SignalAndWaitEvent, Unretained(task_started_events.back().get()),
+             Unretained(&tasks_can_exit_event)));
+  }
+  for (const auto& task_started_event : task_started_events)
+    task_started_event->Wait();
+
+  // Allow tasks to complete their execution and wait to allow workers to
+  // detach.
+  tasks_can_exit_event.Signal();
+  worker_pool_->WaitForAllWorkersIdleForTesting();
+  PlatformThread::Sleep(kReclaimTimeForDetachTests + kExtraTimeToWaitForDetach);
+
+  // Wake up SchedulerWorkers by posting tasks. They should record the
+  // TaskScheduler.NumTasksBetweenWaits.* histogram on wake up.
+  tasks_can_exit_event.Reset();
+  task_started_events.clear();
+  for (int i = 0; i < kNumWorkersInWorkerPool; ++i) {
+    task_started_events.push_back(
+        MakeUnique<WaitableEvent>(WaitableEvent::ResetPolicy::MANUAL,
+                                  WaitableEvent::InitialState::NOT_SIGNALED));
+    task_runner->PostTask(
+        FROM_HERE,
+        Bind(&SignalAndWaitEvent, Unretained(task_started_events.back().get()),
+             Unretained(&tasks_can_exit_event)));
+  }
+  for (const auto& task_started_event : task_started_events)
+    task_started_event->Wait();
+
+  // Verify that counts were recorded to the histogram as expected.
+  // - The "0" bucket has a count of 1 because the SchedulerWorker on top of
+  //   the idle stack wasn't allowed to detach when its sleep timeout expired.
+  //   Instead, it waited on its WaitableEvent again without running a task.
+  EXPECT_EQ(1,
+            num_tasks_between_waits_histogram_->SnapshotSamples()->GetCount(0));
+  // - The "1" bucket has a count of |kNumWorkersInWorkerPool| because each
+  //   SchedulerWorker ran a task before waiting on its WaitableEvent at the
+  //   beginning of the test.
+  EXPECT_EQ(kNumWorkersInWorkerPool,
+            num_tasks_between_waits_histogram_->SnapshotSamples()->GetCount(1));
+  EXPECT_EQ(
+      0, num_tasks_between_waits_histogram_->SnapshotSamples()->GetCount(10));
+
+  tasks_can_exit_event.Signal();
+  worker_pool_->DisallowWorkerDetachmentForTesting();
+}
+
 }  // namespace internal
 }  // namespace base