[TaskScheduler] Do not log 0 to TaskScheduler.NumTasksBetweenWaits histogram on detach.
Doing so accesses |outer_| which was unsafe after invoking Cleanup().
While this will be made safe with https://chromium-review.googlesource.com/c/chromium/src/+/931547
we had noticed this metric being wrong in the past but had postponed
cleaning up to avoid perturbing metrics as part of a bigger change.
Now is the time to drop this report. I don't think we need to rename
the metric because it's an internal diagnosis metric for our team.
R=fdoray@chromium.org, robliao@chromium.org
Bug: 756898, 810464
Change-Id: Ica379d6f3c21aebabeb0574847ba0bfcda346df8
Reviewed-on: https://chromium-review.googlesource.com/931524
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538751}
CrOS-Libchrome-Original-Commit: a8863d2fc9bc0ede0e9f35306ac85cc0195f3e6c
diff --git a/base/task_scheduler/scheduler_worker_pool_impl.cc b/base/task_scheduler/scheduler_worker_pool_impl.cc
index e620c35..00f32c6 100644
--- a/base/task_scheduler/scheduler_worker_pool_impl.cc
+++ b/base/task_scheduler/scheduler_worker_pool_impl.cc
@@ -404,16 +404,6 @@
if (is_on_idle_workers_stack_) {
if (CanCleanupLockRequired(worker))
CleanupLockRequired(worker);
-
- // Since we got here from timing out from the WaitableEvent rather than
- // waking up and completing tasks, we expect to have completed 0 tasks
- // since waiting.
- //
- // TODO(crbug.com/756898): Do not log this histogram when waking up due to
- // timeout.
- DCHECK_EQ(num_tasks_since_last_wait_, 0U);
- outer_->num_tasks_between_waits_histogram_->Add(
- num_tasks_since_last_wait_);
return nullptr;
}
diff --git a/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc b/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
index 072f3ee..ece1f24 100644
--- a/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
@@ -605,7 +605,10 @@
} // namespace
-TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBetweenWaitsWithCleanup) {
+// Verifies that NumTasksBetweenWaits histogram is logged as expected across
+// idle and cleanup periods.
+TEST_F(TaskSchedulerWorkerPoolHistogramTest,
+ NumTasksBetweenWaitsWithIdlePeriodAndCleanup) {
WaitableEvent tasks_can_exit_event(WaitableEvent::ResetPolicy::MANUAL,
WaitableEvent::InitialState::NOT_SIGNALED);
CreateAndStartWorkerPool(kReclaimTimeForCleanupTests,
@@ -628,7 +631,8 @@
task_started_event->Wait();
// Allow tasks to complete their execution and wait to allow workers to
- // cleanup.
+ // cleanup (at least one of them will not cleanup to keep the idle thread
+ // count above zero).
tasks_can_exit_event.Signal();
worker_pool_->WaitForAllWorkersIdleForTesting();
PlatformThread::Sleep(kReclaimTimeForCleanupTests +
@@ -653,15 +657,14 @@
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 cleanup 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(histogram->SnapshotSamples()->GetCount(0), 1);
+ // - The "0" bucket does not have a report because we do not report this
+ // histogram when threads get no work twice in a row and cleanup (or go idle
+ // if last on idle stack).
+ EXPECT_EQ(0, 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.
+ // beginning of the test (counted the same whether resurrecting post-cleanup
+ // or waking from idle).
EXPECT_EQ(static_cast<int>(kNumWorkersInWorkerPool),
histogram->SnapshotSamples()->GetCount(1));
EXPECT_EQ(0, histogram->SnapshotSamples()->GetCount(10));