[TaskScheduler]: Add histogram for number of worker in WorkerPool.

This CL adds TaskScheduler.NumWorkers histogram to measure number of threads
created in each WorkerPool every hour.

Bug: 847501
Change-Id: I5e69f60320231c6ab60d62adb986eecacb15778e
Reviewed-on: https://chromium-review.googlesource.com/1164242
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: Robert Kaplow (slow) <rkaplow@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582724}

base/task/task_scheduler/scheduler_worker_pool_impl.cc

@@ -49,6 +49,7 @@ constexpr char kNumTasksBeforeDetachHistogramPrefix[] =
     "TaskScheduler.NumTasksBeforeDetach.";
 constexpr char kNumTasksBetweenWaitsHistogramPrefix[] =
     "TaskScheduler.NumTasksBetweenWaits.";
+constexpr char kNumThreadsHistogramPrefix[] = "TaskScheduler.NumWorkers.";
 constexpr size_t kMaxNumberOfWorkers = 256;
 
 // Only used in DCHECKs.
@@ -196,6 +197,18 @@ SchedulerWorkerPoolImpl::SchedulerWorkerPoolImpl(
           100,
           50,
           HistogramBase::kUmaTargetedHistogramFlag)),
+      // Mimics the UMA_HISTOGRAM_COUNTS_100 macro. A SchedulerWorkerPool is
+      // expected to run between zero and a few tens of workers.
+      // When it runs more than 100 worker, there is no need to know the exact
+      // number of workers that ran.
+      num_workers_histogram_(Histogram::FactoryGet(
+          JoinString(
+              {kNumThreadsHistogramPrefix, histogram_label, kPoolNameSuffix},
+              ""),
+          1,
+          100,
+          50,
+          HistogramBase::kUmaTargetedHistogramFlag)),
       tracked_ref_factory_(this) {
   DCHECK(!histogram_label.empty());
   DCHECK(!pool_label_.empty());
@@ -270,6 +283,7 @@ void SchedulerWorkerPoolImpl::GetHistograms(
     std::vector<const HistogramBase*>* histograms) const {
   histograms->push_back(detach_duration_histogram_);
   histograms->push_back(num_tasks_between_waits_histogram_);
+  histograms->push_back(num_workers_histogram_);
 }
 
 int SchedulerWorkerPoolImpl::GetMaxConcurrentNonBlockedTasksDeprecated() const {
@@ -360,6 +374,11 @@ void SchedulerWorkerPoolImpl::MaximizeMayBlockThresholdForTesting() {
   maximum_blocked_threshold_for_testing_.Set();
 }
 
+void SchedulerWorkerPoolImpl::RecordNumWorkersHistogram() const {
+  AutoSchedulerLock auto_lock(lock_);
+  num_workers_histogram_->Add(workers_.size());
+}
+
 SchedulerWorkerPoolImpl::SchedulerWorkerDelegateImpl::
     SchedulerWorkerDelegateImpl(TrackedRef<SchedulerWorkerPoolImpl> outer)
     : outer_(std::move(outer)) {

base/task/task_scheduler/scheduler_worker_pool_impl.h

@@ -106,6 +106,10 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
     return num_tasks_between_waits_histogram_;
   }
 
+  const HistogramBase* num_workers_histogram() const {
+    return num_workers_histogram_;
+  }
+
   void GetHistograms(std::vector<const HistogramBase*>* histograms) const;
 
   // Returns the maximum number of non-blocked tasks that can run concurrently
@@ -141,6 +145,9 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Sets the MayBlock waiting threshold to TimeDelta::Max().
   void MaximizeMayBlockThresholdForTesting();
 
+  // Records number of worker.
+  void RecordNumWorkersHistogram() const;
+
  private:
   class SchedulerWorkerDelegateImpl;
 
@@ -334,6 +341,10 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Intentionally leaked.
   HistogramBase* const num_tasks_between_waits_histogram_;
 
+  // TaskScheduler.NumWorkers.[worker pool name] histogram.
+  // Intentionally leaked.
+  HistogramBase* const num_workers_histogram_;
+
   scoped_refptr<TaskRunner> service_thread_task_runner_;
 
   // Optional observer notified when a worker enters and exits its main

base/task/task_scheduler/scheduler_worker_pool_impl_unittest.cc

@@ -35,6 +35,7 @@
 #include "base/task_runner.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/gtest_util.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
@@ -1700,5 +1701,13 @@ TEST_F(TaskSchedulerWorkerPoolImplStartInBodyTest, RacyCleanup) {
   worker_pool_.reset();
 }
 
+TEST_P(TaskSchedulerWorkerPoolImplTestParam, RecordNumWorkersHistogram) {
+  HistogramTester tester;
+  worker_pool_->RecordNumWorkersHistogram();
+  EXPECT_FALSE(
+      tester.GetAllSamples("TaskScheduler.NumWorkers.TestWorkerPoolPool")
+          .empty());
+}
+
 }  // namespace internal
 }  // namespace base

base/task/task_scheduler/service_thread.cc

@@ -23,8 +23,14 @@ TimeDelta g_heartbeat_for_testing = TimeDelta();
 
 }  // namespace
 
-ServiceThread::ServiceThread(const TaskTracker* task_tracker)
-    : Thread("TaskSchedulerServiceThread"), task_tracker_(task_tracker) {}
+ServiceThread::ServiceThread(const TaskTracker* task_tracker,
+                             RepeatingClosure report_heartbeat_metrics_callback)
+    : Thread("TaskSchedulerServiceThread"),
+      task_tracker_(task_tracker),
+      report_heartbeat_metrics_callback_(
+          std::move(report_heartbeat_metrics_callback)) {}
+
+ServiceThread::~ServiceThread() = default;
 
 // static
 void ServiceThread::SetHeartbeatIntervalForTesting(TimeDelta heartbeat) {
@@ -35,17 +41,17 @@ void ServiceThread::Init() {
   // In unit tests we sometimes do not have a fully functional TaskScheduler
   // environment, do not perform the heartbeat report in that case since it
   // relies on such an environment.
-  if (task_tracker_ && TaskScheduler::GetInstance()) {
+  if (TaskScheduler::GetInstance()) {
     // Compute the histogram every hour (with a slight offset to drift if that
     // hour tick happens to line up with specific events). Once per hour per
     // user was deemed sufficient to gather a reliable metric.
     constexpr TimeDelta kHeartbeat = TimeDelta::FromMinutes(59);
 
-    heartbeat_latency_timer_.Start(
+    heartbeat_metrics_timer_.Start(
         FROM_HERE,
         g_heartbeat_for_testing.is_zero() ? kHeartbeat
                                           : g_heartbeat_for_testing,
-        BindRepeating(&ServiceThread::PerformHeartbeatLatencyReport,
+        BindRepeating(&ServiceThread::ReportHeartbeatMetrics,
                       Unretained(this)));
   }
 }
@@ -56,7 +62,15 @@ NOINLINE void ServiceThread::Run(RunLoop* run_loop) {
   base::debug::Alias(&line_number);
 }
 
+void ServiceThread::ReportHeartbeatMetrics() const {
+  report_heartbeat_metrics_callback_.Run();
+  PerformHeartbeatLatencyReport();
+}
+
 void ServiceThread::PerformHeartbeatLatencyReport() const {
+  if (!task_tracker_)
+    return;
+
   static constexpr TaskTraits kReportedTraits[] = {
       {TaskPriority::BEST_EFFORT},   {TaskPriority::BEST_EFFORT, MayBlock()},
       {TaskPriority::USER_VISIBLE},  {TaskPriority::USER_VISIBLE, MayBlock()},

base/task/task_scheduler/service_thread.h

@@ -24,11 +24,15 @@ class TaskTracker;
 // and make it easier to identify the service thread in stack traces.
 class BASE_EXPORT ServiceThread : public Thread {
  public:
-  // Constructs a ServiceThread which will report latency metrics through
-  // |task_tracker| if non-null. In that case, this ServiceThread will assume a
-  // registered TaskScheduler instance and that |task_tracker| will outlive this
-  // ServiceThread.
-  explicit ServiceThread(const TaskTracker* task_tracker);
+  // Constructs a ServiceThread which will record heartbeat metrics. This
+  // includes metrics recorded through |report_heartbeat_metrics_callback|,
+  // in addition to latency metrics through |task_tracker| if non-null. In that
+  // case, this ServiceThread will assume a registered TaskScheduler instance
+  // and that |task_tracker| will outlive this ServiceThread.
+  explicit ServiceThread(const TaskTracker* task_tracker,
+                         RepeatingClosure report_heartbeat_metrics_callback);
+
+  ~ServiceThread() override;
 
   // Overrides the default interval at which |heartbeat_latency_timer_| fires.
   // Call this with a |heartbeat| of zero to undo the override.
@@ -40,16 +44,20 @@ class BASE_EXPORT ServiceThread : public Thread {
   void Init() override;
   void Run(RunLoop* run_loop) override;
 
+  void ReportHeartbeatMetrics() const;
+
   // Kicks off a single async task which will record a histogram on the latency
   // of a randomly chosen set of TaskTraits.
   void PerformHeartbeatLatencyReport() const;
 
   const TaskTracker* const task_tracker_;
 
-  // Fires a recurring heartbeat task to record latency histograms which are
-  // independent from any execution sequence. This is done on the service thread
-  // to avoid all external dependencies (even main thread).
-  base::RepeatingTimer heartbeat_latency_timer_;
+  // Fires a recurring heartbeat task to record metrics which are independent
+  // from any execution sequence. This is done on the service thread to avoid
+  // all external dependencies (even main thread).
+  base::RepeatingTimer heartbeat_metrics_timer_;
+
+  RepeatingClosure report_heartbeat_metrics_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceThread);
 };

base/task/task_scheduler/service_thread_unittest.cc

@@ -43,7 +43,7 @@ void VerifyHasStringOnStack(const std::string& query) {
 #endif
 
 TEST(TaskSchedulerServiceThreadTest, MAYBE_StackHasIdentifyingFrame) {
-  ServiceThread service_thread(nullptr);
+  ServiceThread service_thread(nullptr, DoNothing());
   service_thread.Start();
 
   service_thread.task_runner()->PostTask(

base/task/task_scheduler/task_scheduler_impl.cc

@@ -8,6 +8,8 @@
 #include <string>
 #include <utility>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial_params.h"
@@ -34,7 +36,10 @@ TaskSchedulerImpl::TaskSchedulerImpl(
     StringPiece histogram_label,
     std::unique_ptr<TaskTrackerImpl> task_tracker)
     : task_tracker_(std::move(task_tracker)),
-      service_thread_(std::make_unique<ServiceThread>(task_tracker_.get())),
+      service_thread_(std::make_unique<ServiceThread>(
+          task_tracker_.get(),
+          BindRepeating(&TaskSchedulerImpl::ReportHeartbeatMetrics,
+                        Unretained(this)))),
       single_thread_task_runner_manager_(task_tracker_->GetTrackedRef(),
                                          &delayed_task_manager_) {
   DCHECK(!histogram_label.empty());
@@ -276,5 +281,10 @@ TaskTraits TaskSchedulerImpl::SetUserBlockingPriorityIfNeeded(
              : traits;
 }
 
+void TaskSchedulerImpl::ReportHeartbeatMetrics() const {
+  for (const auto& worker_pool : worker_pools_)
+    worker_pool->RecordNumWorkersHistogram();
+}
+
 }  // namespace internal
 }  // namespace base

base/task/task_scheduler/task_scheduler_impl.h

@@ -97,6 +97,8 @@ class BASE_EXPORT TaskSchedulerImpl : public TaskScheduler {
   // |all_tasks_user_blocking_| is set.
   TaskTraits SetUserBlockingPriorityIfNeeded(const TaskTraits& traits) const;
 
+  void ReportHeartbeatMetrics() const;
+
   const std::unique_ptr<TaskTrackerImpl> task_tracker_;
   std::unique_ptr<Thread> service_thread_;
   DelayedTaskManager delayed_task_manager_;

tools/metrics/histograms/histograms.xml

@@ -104628,6 +104628,16 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram base="true" name="TaskScheduler.NumWorkers" units="workers">
+  <owner>etiennep@chromium.org</owner>
+  <owner>fdoray@chromium.org</owner>
+  <owner>gab@chromium.org</owner>
+  <summary>
+    Number of workers that run in a given SchedulerWorkerPool. Recorded every 59
+    minutes (sampling rate is not expected to affect the distribution).
+  </summary>
+</histogram>
+
 <histogram base="true" name="TaskScheduler.TaskLatency" units="ms">
   <obsolete>
     Deprecated 4/2017. Units changed from milliseconds to microseconds.
@@ -128128,6 +128138,7 @@ uploading your change for review.
   <affected-histogram name="TaskScheduler.HeartbeatLatencyMicroseconds"/>
   <affected-histogram name="TaskScheduler.NumTasksBeforeDetach"/>
   <affected-histogram name="TaskScheduler.NumTasksBetweenWaits"/>
+  <affected-histogram name="TaskScheduler.NumWorkers"/>
   <affected-histogram name="TaskScheduler.TaskLatencyMicroseconds"/>
 </histogram_suffixes>
 
@@ -128268,6 +128279,9 @@ uploading your change for review.
   <affected-histogram name="TaskScheduler.NumTasksBetweenWaits.Browser"/>
   <affected-histogram name="TaskScheduler.NumTasksBetweenWaits.ContentChild"/>
   <affected-histogram name="TaskScheduler.NumTasksBetweenWaits.Renderer"/>
+  <affected-histogram name="TaskScheduler.NumWorkers.Browser"/>
+  <affected-histogram name="TaskScheduler.NumWorkers.ContentChild"/>
+  <affected-histogram name="TaskScheduler.NumWorkers.Renderer"/>
   <affected-histogram name="TaskScheduler.TaskLatency">
     <obsolete>
       Deprecated 12/2016. Pool name removed from task latency histogram name.