[LongTasks] Change |enabled_| flag in PerformanceMonitor

Currently, |enabled_| is a boolean used as a performance optimization to
avoid doing work when nothing is subscribing to violations that are
reported by the PerformanceMonitor. This is often the case, so the
optimization works well, although it is only slightly faster than just
checking whether the corresponding |thresholds_| entry is_zero().

However, we plan to start buffering longtasks soon, which requires
observing for them from the beginning regardless of the presence of
a PerformanceObserver. With this change, |enabled_| would not work well
as an optimization because it would be set to true most of the time.
This CL changes its meaning so that it excludes longtasks, thus being
false most of the time even after we start observing longtasks by
default.

Bug: 1016815
Change-Id: I06d8e8631c83663ead48f344ca08759472edf61c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1874048
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709066}

third_party/blink/renderer/core/frame/performance_monitor.cc

@@ -24,8 +24,11 @@ namespace blink {
 // static
 base::TimeDelta PerformanceMonitor::Threshold(ExecutionContext* context,
                                               Violation violation) {
+  // Calling InstrumentingMonitorExcludingLongTasks wouldn't work properly if
+  // this query is for longtasks.
+  DCHECK(violation != kLongTask);
   PerformanceMonitor* monitor =
-      PerformanceMonitor::InstrumentingMonitor(context);
+      PerformanceMonitor::InstrumentingMonitorExcludingLongTasks(context);
   return monitor ? monitor->thresholds_[violation] : base::TimeDelta();
 }
 
@@ -36,8 +39,11 @@ void PerformanceMonitor::ReportGenericViolation(
     const String& text,
     base::TimeDelta time,
     std::unique_ptr<SourceLocation> location) {
+  // Calling InstrumentingMonitorExcludingLongTasks wouldn't work properly if
+  // this is a longtask violation.
+  DCHECK(violation != kLongTask);
   PerformanceMonitor* monitor =
-      PerformanceMonitor::InstrumentingMonitor(context);
+      PerformanceMonitor::InstrumentingMonitorExcludingLongTasks(context);
   if (!monitor)
     return;
   monitor->InnerReportGenericViolation(context, violation, text, time,
@@ -57,7 +63,7 @@ PerformanceMonitor* PerformanceMonitor::Monitor(
 }
 
 // static
-PerformanceMonitor* PerformanceMonitor::InstrumentingMonitor(
+PerformanceMonitor* PerformanceMonitor::InstrumentingMonitorExcludingLongTasks(
     const ExecutionContext* context) {
   PerformanceMonitor* monitor = PerformanceMonitor::Monitor(context);
   return monitor && monitor->enabled_ ? monitor : nullptr;
@@ -116,8 +122,13 @@ void PerformanceMonitor::UpdateInstrumentation() {
     }
   }
 
-  enabled_ = std::count(std::begin(thresholds_), std::end(thresholds_),
-                        base::TimeDelta()) < static_cast<int>(kAfterLast);
+  static_assert(kLongTask == 0u,
+                "kLongTask should be the first value in Violation for the "
+                "|enabled_| definition below to be correct");
+  // Since kLongTask is the first in |thresholds_|, we count from one after
+  // begin(thresholds_).
+  enabled_ = std::count(std::begin(thresholds_) + 1, std::end(thresholds_),
+                        base::TimeDelta()) < static_cast<int>(kAfterLast) - 1;
 }
 
 void PerformanceMonitor::WillExecuteScript(ExecutionContext* context) {
@@ -157,13 +168,15 @@ void PerformanceMonitor::UpdateTaskShouldBeReported(LocalFrame* frame) {
 void PerformanceMonitor::Will(const probe::RecalculateStyle& probe) {
   UpdateTaskShouldBeReported(probe.document ? probe.document->GetFrame()
                                             : nullptr);
-  if (enabled_ && !thresholds_[kLongLayout].is_zero() && script_depth_)
+  if (enabled_ && !thresholds_[kLongLayout].is_zero() && script_depth_) {
     probe.CaptureStartTime();
+  }
 }
 
 void PerformanceMonitor::Did(const probe::RecalculateStyle& probe) {
-  if (enabled_ && script_depth_ && !thresholds_[kLongLayout].is_zero())
+  if (enabled_ && script_depth_ && !thresholds_[kLongLayout].is_zero()) {
     per_task_style_and_layout_time_ += probe.Duration();
+  }
 }
 
 void PerformanceMonitor::Will(const probe::UpdateLayout& probe) {
@@ -258,6 +271,8 @@ void PerformanceMonitor::DocumentWriteFetchScript(Document* document) {
 void PerformanceMonitor::WillProcessTask(base::TimeTicks start_time) {
   // Reset m_taskExecutionContext. We don't clear this in didProcessTask
   // as it is needed in ReportTaskTime which occurs after didProcessTask.
+  // Always reset variables needed for longtasks, regardless of the value of
+  // |enabled_|.
   task_execution_context_ = nullptr;
   task_has_multiple_contexts_ = false;
   task_should_be_reported_ = false;
@@ -274,8 +289,30 @@ void PerformanceMonitor::WillProcessTask(base::TimeTicks start_time) {
 
 void PerformanceMonitor::DidProcessTask(base::TimeTicks start_time,
                                         base::TimeTicks end_time) {
-  if (!enabled_ || !task_should_be_reported_)
+  if (!task_should_be_reported_)
     return;
+
+  // Do not check the value of |enabled_| before processing longtasks.
+  // |enabled_| can be false while there are subscriptions to longtask
+  // violations.
+  if (!thresholds_[kLongTask].is_zero()) {
+    base::TimeDelta task_time = end_time - start_time;
+    if (task_time > thresholds_[kLongTask]) {
+      ClientThresholds* client_thresholds = subscriptions_.at(kLongTask);
+      for (const auto& it : *client_thresholds) {
+        if (it.value < task_time) {
+          it.key->ReportLongTask(
+              start_time, end_time,
+              task_has_multiple_contexts_ ? nullptr : task_execution_context_,
+              task_has_multiple_contexts_);
+        }
+      }
+    }
+  }
+
+  if (!enabled_)
+    return;
+
   base::TimeDelta layout_threshold = thresholds_[kLongLayout];
   base::TimeDelta layout_time = per_task_style_and_layout_time_;
   if (!layout_threshold.is_zero() && layout_time > layout_threshold) {
@@ -286,19 +323,6 @@ void PerformanceMonitor::DidProcessTask(base::TimeTicks start_time,
         it.key->ReportLongLayout(layout_time);
     }
   }
-
-  base::TimeDelta task_time = end_time - start_time;
-  if (!thresholds_[kLongTask].is_zero() && task_time > thresholds_[kLongTask]) {
-    ClientThresholds* client_thresholds = subscriptions_.at(kLongTask);
-    for (const auto& it : *client_thresholds) {
-      if (it.value < task_time) {
-        it.key->ReportLongTask(
-            start_time, end_time,
-            task_has_multiple_contexts_ ? nullptr : task_execution_context_,
-            task_has_multiple_contexts_);
-      }
-    }
-  }
 }
 
 void PerformanceMonitor::InnerReportGenericViolation(

third_party/blink/renderer/core/frame/performance_monitor.h

@@ -42,7 +42,7 @@ class CORE_EXPORT PerformanceMonitor final
       public base::sequence_manager::TaskTimeObserver {
  public:
   enum Violation : size_t {
-    kLongTask,
+    kLongTask = 0,
     kLongLayout,
     kBlockedEvent,
     kBlockedParser,
@@ -111,7 +111,10 @@ class CORE_EXPORT PerformanceMonitor final
   friend class WindowPerformanceTest;
 
   static PerformanceMonitor* Monitor(const ExecutionContext*);
-  static PerformanceMonitor* InstrumentingMonitor(const ExecutionContext*);
+  // Returns the monitor of the ExecutionContext if its
+  // |enabled_| is set, nullptr otherwise.
+  static PerformanceMonitor* InstrumentingMonitorExcludingLongTasks(
+      const ExecutionContext*);
 
   void UpdateInstrumentation();
 
@@ -136,6 +139,8 @@ class CORE_EXPORT PerformanceMonitor final
       const HeapHashSet<Member<Frame>>& frame_contexts,
       Frame* observer_frame);
 
+  // This boolean is used to track whether there is any subscription to any
+  // Violation other than longtasks.
   bool enabled_ = false;
   base::TimeDelta per_task_style_and_layout_time_;
   unsigned script_depth_ = 0;

third_party/blink/renderer/core/timing/window_performance_test.cc

@@ -41,8 +41,9 @@ class WindowPerformanceTest : public testing::Test {
   }
 
   bool ObservingLongTasks() {
-    return PerformanceMonitor::InstrumentingMonitor(
-        performance_->GetExecutionContext());
+    return !PerformanceMonitor::Monitor(performance_->GetExecutionContext())
+                ->thresholds_[PerformanceMonitor::kLongTask]
+                .is_zero();
   }
 
   void AddLongTaskObserver() {