[scheduler] Add RendererScheduler.TaskDurationPerQueueType.

third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc

Index: third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
diff --git a/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc b/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
index 509b289113297bf853cb7faf50c16645809fbefd..7fbc5350e6904bd88db01b8d66ef6114064ff121 100644
--- a/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
+++ b/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
@@ -8,6 +8,7 @@
 #include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -49,6 +50,9 @@ constexpr base::TimeDelta kThreadLoadTrackerWaitingPeriodBeforeReporting =
 // We do not throttle anything while audio is played and shortly after that.
 constexpr base::TimeDelta kThrottlingDelayAfterAudioIsPlayed =
     base::TimeDelta::FromSeconds(5);
+// Maximal bound on task duration for reporting.
+constexpr base::TimeDelta kMaxTaskDurationForReporting =
+    base::TimeDelta::FromMinutes(1);
 
 void ReportForegroundRendererTaskLoad(base::TimeTicks time, double load) {
   if (!blink::RuntimeEnabledFeatures::timerThrottlingForBackgroundTabsEnabled())
@@ -1778,13 +1782,38 @@ void RendererSchedulerImpl::didProcessTask(TaskQueue* task_queue,
       start_time_ticks, end_time_ticks);
   MainThreadOnly().background_main_thread_load_tracker.RecordTaskTime(
       start_time_ticks, end_time_ticks);
+
   // TODO(altimin): Per-page metrics should also be considered.
-  UMA_HISTOGRAM_CUSTOM_COUNTS(
-      "RendererScheduler.TaskTime",
-      (end_time_ticks - start_time_ticks).InMicroseconds(), 1, 1000000, 50);
+  RecordTaskMetrics(task_queue->GetQueueType(),
+                    end_time_ticks - start_time_ticks);
+}
+
+void RendererSchedulerImpl::RecordTaskMetrics(TaskQueue::QueueType queue_type,
+                                              base::TimeDelta duration) {
+  duration = std::min(duration, kMaxTaskDurationForReporting);
+
+  UMA_HISTOGRAM_CUSTOM_COUNTS("RendererScheduler.TaskTime",
+                              duration.InMicroseconds(), 1, 1000000, 50);

[Ilya Sherman, 2017/03/16 21:23:43]

Optional nit: The constant 1000000 would be easier to read as "1000 * 1000",
IMO.

[altimin, 2017/03/16 23:44:23]

Done.

+
   UMA_HISTOGRAM_ENUMERATION("RendererScheduler.NumberOfTasksPerQueueType",
-                            static_cast<int>(task_queue->GetQueueType()),
+                            static_cast<int>(queue_type),
                             static_cast<int>(TaskQueue::QueueType::COUNT));
+
+  // Report only whole milliseconds to avoid overflow.
+  base::TimeDelta& total_duration =
+      MainThreadOnly()
+          .task_duration_per_queue_type[static_cast<int>(queue_type)];
+  total_duration += duration;
+  int64_t milliseconds = total_duration.InMilliseconds();
+  if (milliseconds > 0) {
+    total_duration -= base::TimeDelta::FromMilliseconds(milliseconds);
+    base::Histogram::FactoryGet("RendererScheduler.TaskDurationPerQueueType", 0,

[Ilya Sherman, 2017/03/16 21:23:43]

The min value should be 1, not 0.

[altimin, 2017/03/16 23:44:22]

Done.

+                                static_cast<int>(TaskQueue::QueueType::COUNT),
+                                static_cast<int>(TaskQueue::QueueType::COUNT),

[Ilya Sherman, 2017/03/16 21:23:43]

This should be COUNT+1 -- the final bucket should not be used.

[altimin, 2017/03/16 23:44:23]

Done. To be clear — if my samples are in range [0..N) I need to pass (1, N, N +
1) as parameters, right?

[Ilya Sherman, 2017/03/16 23:58:45]

Correct.  Yeah, it's a fairly confusing API, sorry!

+                                base::HistogramBase::kUmaTargetedHistogramFlag)
+        ->AddCount(static_cast<int>(queue_type),
+                   static_cast<int>(milliseconds));

[Ilya Sherman, 2017/03/16 21:23:43]

It looks like histograms aggregate values in 32-bit counters.  Did you somehow
verify that overflow is not a concern?

[altimin, 2017/03/16 23:44:22]

By design we're expecting number of samples to be less than 1000 (samples/sec) *
1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number overflows,
we rely on UMA to throw the data out.

[Ilya Sherman, 2017/03/16 23:58:45]

Metrics are not reset on each upload -- we simply take a snapshot, and compute
differences.  So if you're relying on the upload period being a reset of these
metrics, then that won't work.

Also, what do you mean by "we rely on UMA to throw the data out"?  Where do you
expect UMA to do that?

[altimin, 2017/03/17 00:24:00]

That's insightful, thanks. Any chance we could start using 64-bit integers here?

By "throwing out" I meant that I was hoping that when overflow is detected, UMA
will just throw data out.

[Ilya Sherman, 2017/03/17 20:27:36]

I think we'd need to measure whether there are any adverse impacts on memory if
we switch from 32-bit to 64-bit.  If it's memory-neutral, then using 64-bit
integers is probably reasonable.  I don't think anyone on the metrics team is
likely to get to this soon; but if you have time to drive such an effort, we
could help advise you.
UMA doesn't do anything to detect overflow that I know of.  (It's possible that
we do and I don't know of it, though.  Steve was mentioning something that
sounded kind of like this on the email thread.)

[altimin, 2017/03/17 23:49:53]

I took a quick look at this — it seems that absence of 64-bit atomic integers at
some platforms can be a problem too.

So I'd like to suggest a special 64-bit subtype of histogram (templates to the
rescue?) with a comment that it can cause performance and memory overhead. WDYT?

[Ilya Sherman, 2017/03/18 01:09:59]

Hmm, I guess it depends on how much of a maintenance cost it adds.  I'm a bit
hesitant to introduce yet another type of histogram that is slightly different
than others, especially given that the current known use-case requires custom
analysis scripts to work with it.  I'd suggest perhaps bringing this up on the
metrics team mailing list.

I'd still lean toward a solution that uses one histogram per queue type, fwiw.

+  }
 }
 
 void RendererSchedulerImpl::AddTaskTimeObserver(