Add TaskScheduler.NumTasksBeforeDetach.[worker pool name] histogram.

base/task_scheduler/scheduler_worker_pool_impl_unittest.cc

Index: base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
diff --git a/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc b/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
index 0f8475a69051fc7f56a1d5d3f3feb1d4a6a75f39..b67847acb7fd86205308abb0bb6b179bfb1fab09 100644
--- a/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
@@ -593,7 +593,11 @@ class TaskSchedulerWorkerPoolHistogramTest
  protected:
   void SetUp() override {}

[gab, 2016/10/17 18:24:00]

drive-by nit: No-op override?

[robliao, 2016/10/17 23:05:39]

This suppresses the common worker_pool_ init and allow the test case itself to
init it.

[gab, 2016/10/18 14:45:45]

Hmmm okay, then how about a comment stating that this is intentional.

[fdoray, 2016/10/18 19:41:23]

Done.

 
-  void TearDown() override { worker_pool_->JoinForTesting(); }
+  void TearDown() override {
+    // |worker_pool_| initialization is done in test body.
+    if (worker_pool_)
+      worker_pool_->JoinForTesting();

[gab, 2016/10/17 18:24:00]

This code belongs in TaskSchedulerWorkerPoolImplTest::TearDown() IMO as it owns
|worker_pool_|.

Also: overriding TearDown of another fixture should always be followed by
calling the parent class' TearDown method (which doesn't make sense here hence
why there just shouldn't be an override)

[fdoray, 2016/10/18 19:41:23]

Done.

+  }
 
  private:
   std::unique_ptr<StatisticsRecorder> statistics_recorder_ =
@@ -633,15 +637,10 @@ TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBetweenWaits) {
   worker_pool_->WaitForAllWorkersIdleForTesting();
 
   // Verify that counts were recorded to the histogram as expected.
-  EXPECT_EQ(0, worker_pool_->num_tasks_between_waits_histogram_for_testing()
-                   ->SnapshotSamples()
-                   ->GetCount(0));
-  EXPECT_EQ(1, worker_pool_->num_tasks_between_waits_histogram_for_testing()
-                   ->SnapshotSamples()
-                   ->GetCount(3));
-  EXPECT_EQ(0, worker_pool_->num_tasks_between_waits_histogram_for_testing()
-                   ->SnapshotSamples()
-                   ->GetCount(10));
+  const auto* histogram = worker_pool_->num_tasks_between_waits_histogram();
+  EXPECT_EQ(0, histogram->SnapshotSamples()->GetCount(0));
+  EXPECT_EQ(1, histogram->SnapshotSamples()->GetCount(3));
+  EXPECT_EQ(0, histogram->SnapshotSamples()->GetCount(10));
 }
 
 namespace {
@@ -697,31 +696,64 @@ TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBetweenWaitsWithDetach) {
   for (const auto& task_started_event : task_started_events)
     task_started_event->Wait();
 
+  const auto* histogram = worker_pool_->num_tasks_between_waits_histogram();
+
   // Verify that counts were recorded to the histogram as expected.
   // - The "0" bucket has a count of at least 1 because the SchedulerWorker on
   //   top of the idle stack isn't allowed to detach when its sleep timeout
   //   expires. Instead, it waits on its WaitableEvent again without running a
   //   task. The count may be higher than 1 because of spurious wake ups before
   //   the sleep timeout expires.
-  EXPECT_GE(worker_pool_->num_tasks_between_waits_histogram_for_testing()
-                ->SnapshotSamples()
-                ->GetCount(0),
-            1);
+  EXPECT_GE(histogram->SnapshotSamples()->GetCount(0), 1);
   // - 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(static_cast<int>(kNumWorkersInWorkerPool),
-            worker_pool_->num_tasks_between_waits_histogram_for_testing()
-                ->SnapshotSamples()
-                ->GetCount(1));
-  EXPECT_EQ(0, worker_pool_->num_tasks_between_waits_histogram_for_testing()
-                   ->SnapshotSamples()
-                   ->GetCount(10));
+            histogram->SnapshotSamples()->GetCount(1));
+  EXPECT_EQ(0, histogram->SnapshotSamples()->GetCount(10));
 
   tasks_can_exit_event.Signal();
   worker_pool_->WaitForAllWorkersIdleForTesting();
   worker_pool_->DisallowWorkerDetachmentForTesting();
 }
 
+TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBeforeDetach) {
+  InitializeWorkerPool(kReclaimTimeForDetachTests, kNumWorkersInWorkerPool);
+  auto task_runner = worker_pool_->CreateTaskRunnerWithTraits(
+      TaskTraits(), ExecutionMode::SINGLE_THREADED);
+  auto other_task_runner = worker_pool_->CreateTaskRunnerWithTraits(
+      TaskTraits(), ExecutionMode::SINGLE_THREADED);

[gab, 2016/10/17 18:24:00]

This test assumes both of these aren't assigned the same thread. Can you add
something that documents and verifies that (so it's easier to understand what's
going on if we one day change the thread assignment algo and this test starts
failing -- or worst flaking).

[fdoray, 2016/10/18 19:41:22]

Done.

+
+  // Post 3 tasks and wait until they run.
+  task_runner->PostTask(FROM_HERE, Bind(&DoNothing));
+  task_runner->PostTask(FROM_HERE, Bind(&DoNothing));
+  task_runner->PostTask(FROM_HERE, Bind(&DoNothing));
+  worker_pool_->WaitForAllWorkersIdleForTesting();
+
+  // To allow the SchedulerWorker associated with |task_runner| to detach:
+  // - Make sure it isn't on top of the idle stack by waking up another
+  //   SchedulerWorker.
+  // - Release |task_runner|.
+  other_task_runner->PostTask(FROM_HERE, Bind(&DoNothing));
+  task_runner = nullptr;

[gab, 2016/10/17 18:24:00]

nit: I prefer |task_runner.Release()| here to explicitly document that this is
releasing the ref (just like we use .reset() on unique_ptr's)

[fdoray, 2016/10/18 19:41:23]

Doesn't work. Release() is a private static method
https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?l=397

[gab, 2016/10/19 18:35:08]

Ah interesting, somehow was convinced it was exposed. I understand why
Add/Remove refs isn't exposed but thought scoped_refptr<>::Release() would be
exposed as it can't introduce miscounting (i.e. it would both release the ref
and make that pointer null).

Oh well :)

+
+  // Allow the SchedulerWorker to detach.
+  PlatformThread::Sleep(kReclaimTimeForDetachTests + kExtraTimeToWaitForDetach);
+
+  // Join the SchedulerWorkerPool. This forces SchedulerWorkers that detached
+  // during the test to record to the histogram.
+  worker_pool_->WaitForAllWorkersIdleForTesting();
+  worker_pool_->DisallowWorkerDetachmentForTesting();

[gab, 2016/10/17 18:24:00]

Is this required? If we don't post any more tasks, nothing should be awaken and
thus nothing should re-detach, right?

[robliao, 2016/10/17 23:05:39]

It's a nice to have for test cleanliness for the subsequent JoinForTesting.

[gab, 2016/10/18 14:45:46]

IDK, to me any code that should never do anything decreases readability as a
reader has to think why it matters that it's there.

[fdoray, 2016/10/18 19:41:23]

DisallowWorkerDetachmentForTesting() must be called before JoinForTesting() when
the detach time is not TimeDelta::Max()
https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo...

+  worker_pool_->JoinForTesting();
+  const auto* histogram = worker_pool_->num_tasks_before_detach_histogram();
+  other_task_runner = nullptr;

[gab, 2016/10/17 18:24:00]

.Release()

[fdoray, 2016/10/18 19:41:23]

N/A

+  worker_pool_.reset();
+
+  // Verify that counts were recorded to the histogram as expected.
+  EXPECT_EQ(0, histogram->SnapshotSamples()->GetCount(0));
+  EXPECT_EQ(1, histogram->SnapshotSamples()->GetCount(3));
+  EXPECT_EQ(0, histogram->SnapshotSamples()->GetCount(10));
+}
+
 }  // namespace internal
 }  // namespace base