SequenceManager: Only record task time if we need to

We don't need task timing on the UI thread, so this patch arranges for
time only to be sampled if we have an observer that needs it.

Bug: 897751
Change-Id: I5915efdb903813a574ca05cee316cb30fa75b39c
Reviewed-on: https://chromium-review.googlesource.com/c/1297425
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603059}

base/task/sequence_manager/sequence_manager_impl.cc

@@ -474,18 +474,24 @@ void SequenceManagerImpl::WillQueueTask(Task* pending_task) {
 
 TaskQueue::TaskTiming SequenceManagerImpl::InitializeTaskTiming(
     internal::TaskQueueImpl* task_queue) {
-  bool records_wall_time =
-      (task_queue->GetShouldNotifyObservers() &&
-       main_thread_only().task_time_observers.might_have_observers()) ||
-      task_queue->RequiresTaskTiming();
+  bool records_wall_time = ShouldRecordTaskTiming(task_queue);
   bool records_thread_time = records_wall_time && ShouldRecordCPUTimeForTask();
   return TaskQueue::TaskTiming(records_wall_time, records_thread_time);
 }
 
+bool SequenceManagerImpl::ShouldRecordTaskTiming(
+    const internal::TaskQueueImpl* task_queue) {
+  if (task_queue->RequiresTaskTiming())
+    return true;
+  return main_thread_only().nesting_depth == 0 &&
+         main_thread_only().task_time_observers.might_have_observers();
+}
+
 void SequenceManagerImpl::NotifyWillProcessTask(ExecutingTask* executing_task,
                                                 LazyNow* time_before_task) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("sequence_manager"),
                "SequenceManagerImpl::NotifyWillProcessTaskObservers");
+
   if (executing_task->task_queue->GetQuiescenceMonitored())
     main_thread_only().task_was_run_on_quiescence_monitored_queue = true;
 
@@ -498,7 +504,9 @@ void SequenceManagerImpl::NotifyWillProcessTask(ExecutingTask* executing_task,
       executing_task->pending_task.posted_from.function_name());
 #endif  // OS_NACL
 
-  executing_task->task_timing.RecordTaskStart(time_before_task);
+  bool record_task_timing = ShouldRecordTaskTiming(executing_task->task_queue);
+  if (record_task_timing)
+    executing_task->task_timing.RecordTaskStart(time_before_task);
 
   if (!executing_task->task_queue->GetShouldNotifyObservers())
     return;
@@ -517,11 +525,7 @@ void SequenceManagerImpl::NotifyWillProcessTask(ExecutingTask* executing_task,
         executing_task->pending_task);
   }
 
-  bool notify_time_observers =
-      main_thread_only().task_time_observers.might_have_observers() ||
-      executing_task->task_queue->RequiresTaskTiming();
-
-  if (!notify_time_observers)
+  if (!record_task_timing)
     return;
 
   if (main_thread_only().nesting_depth == 0) {
@@ -543,13 +547,16 @@ void SequenceManagerImpl::NotifyDidProcessTask(ExecutingTask* executing_task,
                                                LazyNow* time_after_task) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("sequence_manager"),
                "SequenceManagerImpl::NotifyDidProcessTaskObservers");
+  if (!executing_task->task_queue->GetShouldNotifyObservers())
+    return;
 
-  executing_task->task_timing.RecordTaskEnd(time_after_task);
+  bool record_task_timing = ShouldRecordTaskTiming(executing_task->task_queue);
 
-  const TaskQueue::TaskTiming& task_timing = executing_task->task_timing;
+  // Record end time ASAP to avoid bias due to the overhead of observers.
+  if (record_task_timing)
+    executing_task->task_timing.RecordTaskEnd(time_after_task);
 
-  if (!executing_task->task_queue->GetShouldNotifyObservers())
-    return;
+  const TaskQueue::TaskTiming& task_timing = executing_task->task_timing;
 
   if (task_timing.has_wall_time() && main_thread_only().nesting_depth == 0) {
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("sequence_manager"),

base/task/sequence_manager/sequence_manager_impl.h

@@ -313,6 +313,7 @@ class BASE_EXPORT SequenceManagerImpl
   // Deletes queues marked for deletion and empty queues marked for shutdown.
   void CleanUpQueues();
 
+  bool ShouldRecordTaskTiming(const internal::TaskQueueImpl* task_queue);
   bool ShouldRecordCPUTimeForTask();
 
   // Helper to terminate all scoped trace events to allow starting new ones

base/task/sequence_manager/sequence_manager_perftest.cc

@@ -104,7 +104,6 @@ class SequenceManagerPerfTest : public testing::TestWithParam<PerfTestType> {
     if (manager_) {
       time_domain_ = std::make_unique<PerfTestTimeDomain>();
       manager_->RegisterTimeDomain(time_domain_.get());
-      manager_->AddTaskTimeObserver(&test_task_time_observer_);
     }
   }
 
@@ -336,9 +335,6 @@ class SequenceManagerPerfTest : public testing::TestWithParam<PerfTestType> {
 
   RepeatingClosure delayed_task_closure_;
   RepeatingClosure immediate_task_closure_;
-  // TODO(alexclarke): parameterize so we can measure with and without a
-  // TaskTimeObserver.
-  TestTaskTimeObserver test_task_time_observer_;
 };
 
 INSTANTIATE_TEST_CASE_P(

base/task/sequence_manager/task_queue_impl.cc

@@ -961,6 +961,7 @@ bool TaskQueueImpl::HasPendingImmediateWork() {
 
 void TaskQueueImpl::SetOnTaskStartedHandler(
     TaskQueueImpl::OnTaskStartedHandler handler) {
+  DCHECK(should_notify_observers_ || handler.is_null());
   main_thread_only().on_task_started_handler = std::move(handler);
 }
 
@@ -972,6 +973,7 @@ void TaskQueueImpl::OnTaskStarted(const Task& task,
 
 void TaskQueueImpl::SetOnTaskCompletedHandler(
     TaskQueueImpl::OnTaskCompletedHandler handler) {
+  DCHECK(should_notify_observers_ || handler.is_null());
   main_thread_only().on_task_completed_handler = std::move(handler);
 }
 

third_party/blink/renderer/platform/scheduler/main_thread/main_thread_task_queue.cc

@@ -115,7 +115,7 @@ MainThreadTaskQueue::MainThreadTaskQueue(
       freeze_when_keep_active_(params.freeze_when_keep_active),
       main_thread_scheduler_(main_thread_scheduler),
       frame_scheduler_(params.frame_scheduler) {
-  if (GetTaskQueueImpl()) {
+  if (GetTaskQueueImpl() && spec.should_notify_observers) {
     // TaskQueueImpl may be null for tests.
     // TODO(scheduler-dev): Consider mapping directly to
     // MainThreadSchedulerImpl::OnTaskStarted/Completed. At the moment this

third_party/blink/renderer/platform/scheduler/worker/non_main_thread_task_queue.cc

@@ -17,7 +17,7 @@ NonMainThreadTaskQueue::NonMainThreadTaskQueue(
     NonMainThreadSchedulerImpl* non_main_thread_scheduler)
     : TaskQueue(std::move(impl), spec),
       non_main_thread_scheduler_(non_main_thread_scheduler) {
-  if (GetTaskQueueImpl()) {
+  if (GetTaskQueueImpl() && spec.should_notify_observers) {
     // TaskQueueImpl may be null for tests.
     GetTaskQueueImpl()->SetOnTaskCompletedHandler(base::BindRepeating(
         &NonMainThreadTaskQueue::OnTaskCompleted, base::Unretained(this)));