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

base/task_scheduler/scheduler_worker_pool_impl.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 <algorithm>
 #include <utility>
@@ skipping 16 matching lines @@
 #include "base/time/time.h"
 
 namespace base {
 namespace internal {
 
 namespace {
 
 constexpr char kPoolNameSuffix[] = "Pool";
 constexpr char kDetachDurationHistogramPrefix[] =
     "TaskScheduler.DetachDuration.";
+constexpr char kNumTasksBeforeDetachHistogramPrefix[] =
+    "TaskScheduler.NumTasksBeforeDetach.";
 constexpr char kNumTasksBetweenWaitsHistogramPrefix[] =
     "TaskScheduler.NumTasksBetweenWaits.";
 constexpr char kTaskLatencyHistogramPrefix[] = "TaskScheduler.TaskLatency.";
 
 // SchedulerWorker that owns the current thread, if any.
 LazyInstance<ThreadLocalPointer<const SchedulerWorker>>::Leaky
     tls_current_worker = LAZY_INSTANCE_INITIALIZER;
 
 // SchedulerWorkerPool that owns the current thread, if any.
 LazyInstance<ThreadLocalPointer<const SchedulerWorkerPool>>::Leaky
@@ skipping 226 matching lines @@
   bool last_get_work_returned_nullptr_ = false;
 
   // Indicates whether the SchedulerWorker was detached since the last call to
   // GetWork().
   bool did_detach_since_last_get_work_ = false;
 
   // Number of tasks executed since the last time the
   // TaskScheduler.NumTasksBetweenWaits histogram was recorded.
   size_t num_tasks_since_last_wait_ = 0;
 
+  // Number of tasks executed since the last time the
+  // TaskScheduler.NumTasksBeforeDetach histogram was recorded.
+  size_t num_tasks_since_last_detach_ = 0;
+
   subtle::Atomic32 num_single_threaded_runners_ = 0;
 
   const int index_;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulerWorkerDelegateImpl);
 };
 
 SchedulerWorkerPoolImpl::~SchedulerWorkerPoolImpl() {
   // SchedulerWorkerPool should never be deleted in production unless its
   // initialization failed.
@@ skipping 186 matching lines @@
     const TimeDelta& detach_duration) {
 #if DCHECK_IS_ON()
   // Wait for |outer_->workers_created_| to avoid traversing
   // |outer_->workers_| while it is being filled by Initialize().
   outer_->workers_created_.Wait();
   DCHECK(ContainsWorker(outer_->workers_, worker));
 #endif
 
   DCHECK_EQ(num_tasks_since_last_wait_, 0U);
 
+  // Record histograms if the worker detached in the past.
   if (!detach_duration.is_max()) {
     outer_->detach_duration_histogram_->AddTime(detach_duration);
+    outer_->num_tasks_before_detach_histogram_->Add(

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

Recording here will only record if the thread is recreated. This seems weird to
me. Why not record on detach?

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

The SchedulerWorker is free to ignore the detach request (e.g. race condition
where a wakeup is pending while the scheduler worker gets ready to detach).

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

Right but we could still add SchedulerWorkerDelegate::OnDetach() and call it at
the end of a successful SchedulerWorker::Detach().

The ideal detach is one where we never re-attach and in this case the metric
will ignore those and thus be incorrectly skewed IMO.

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

Done.

+        num_tasks_since_last_detach_);
+    num_tasks_since_last_detach_ = 0;
     did_detach_since_last_get_work_ = true;
   }
 
   PlatformThread::SetName(
       StringPrintf("TaskScheduler%sWorker%d", outer_->name_.c_str(), index_));
 
   DCHECK(!tls_current_worker.Get().Get());
   DCHECK(!tls_current_worker_pool.Get().Get());
   tls_current_worker.Get().Set(worker);
   tls_current_worker_pool.Get().Set(outer_);
@@ skipping 85 matching lines @@
   did_detach_since_last_get_work_ = false;
   last_get_work_returned_nullptr_ = false;
 
   return sequence;
 }
 
 void SchedulerWorkerPoolImpl::SchedulerWorkerDelegateImpl::
     DidRunTaskWithPriority(TaskPriority task_priority,
                            const TimeDelta& task_latency) {
   ++num_tasks_since_last_wait_;
+  ++num_tasks_since_last_detach_;
 
   const int priority_index = static_cast<int>(task_priority);
 
   // As explained in the header file, histograms are allocated on demand. It
   // doesn't matter if an element of |task_latency_histograms_| is set multiple
   // times since GetTaskLatencyHistogram() is idempotent. As explained in the
   // comment at the top of histogram_macros.h, barriers are required.
   HistogramBase* task_latency_histogram = reinterpret_cast<HistogramBase*>(
       subtle::Acquire_Load(&outer_->task_latency_histograms_[priority_index]));
   if (!task_latency_histogram) {
@@ skipping 60 matching lines @@
       workers_created_(WaitableEvent::ResetPolicy::MANUAL,
                        WaitableEvent::InitialState::NOT_SIGNALED),
 #endif
       // Mimics the UMA_HISTOGRAM_LONG_TIMES macro.
       detach_duration_histogram_(Histogram::FactoryTimeGet(
           kDetachDurationHistogramPrefix + name_ + kPoolNameSuffix,
           TimeDelta::FromMilliseconds(1),
           TimeDelta::FromHours(1),
           50,
           HistogramBase::kUmaTargetedHistogramFlag)),
+      // Mimics the UMA_HISTOGRAM_COUNTS_1000 macro. A SchedulerWorker is
+      // expected to run between zero and a few hundreds of tasks before
+      // detaching. When it runs more than 1000 tasks, there is no need to know
+      // the exact number of tasks that ran.

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

While I agree that once it runs more than a 1000 tasks we can call that a "good
run for the money" and not care how many exactly (i.e. it was worth it to wake
it up).

I don't think it's true that "a SW is expected to run between zero and a few
hundreds of tasks before detaching".

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

Done.

+      num_tasks_before_detach_histogram_(Histogram::FactoryGet(
+          kNumTasksBeforeDetachHistogramPrefix + name_ + kPoolNameSuffix,
+          1,
+          1000,
+          50,
+          HistogramBase::kUmaTargetedHistogramFlag)),
       // Mimics the UMA_HISTOGRAM_COUNTS_100 macro. A SchedulerWorker is
       // expected to run between zero and a few tens of tasks between waits.
       // When it runs more than 100 tasks, there is no need to know the exact
       // number of tasks that ran.
       num_tasks_between_waits_histogram_(Histogram::FactoryGet(
           kNumTasksBetweenWaitsHistogramPrefix + name_ + kPoolNameSuffix,
           1,
           100,
           50,
           HistogramBase::kUmaTargetedHistogramFlag)),
@@ skipping 79 matching lines @@
   AutoSchedulerLock auto_lock(idle_workers_stack_lock_);
   idle_workers_stack_.Remove(worker);
 }
 
 bool SchedulerWorkerPoolImpl::CanWorkerDetachForTesting() {
   return !worker_detachment_disallowed_.IsSet();
 }
 
 }  // namespace internal
 }  // namespace base