Add TaskScheduler.NumTasksBeforeDetach.[worker pool name] 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>
@@ skipping 573 matching lines @@
 }
 
 namespace {
 
 class TaskSchedulerWorkerPoolHistogramTest
     : public TaskSchedulerWorkerPoolImplTest {
  public:
   TaskSchedulerWorkerPoolHistogramTest() = default;
 
  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_ =
       StatisticsRecorder::CreateTemporaryForTesting();
 
  private:

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

drive-by nit: double private

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

Done.

   DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerPoolHistogramTest);
 };
 
 }  // namespace
 
 TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBetweenWaits) {
   WaitableEvent event(WaitableEvent::ResetPolicy::MANUAL,
                       WaitableEvent::InitialState::NOT_SIGNALED);
   InitializeWorkerPool(TimeDelta::Max(), kNumWorkersInWorkerPool);
   auto task_runner = worker_pool_->CreateTaskRunnerWithTraits(
@@ skipping 13 matching lines @@
   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, 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 {
 
 void SignalAndWaitEvent(WaitableEvent* signal_event,
                         WaitableEvent* wait_event) {
   signal_event->Signal();
   wait_event->Wait();
 }
 
@@ skipping 35 matching lines @@
         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();
 
+  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