Change FMP swap timestamp to reflect swap time of FMP candidate

The swap-promise used to compute the GPU swap timestamp for FMP was not queued
until SetFirstMeaninfulPaint is called, but this method is called with a
timestamp that was computed before the network quiet timer fired and FMP was
decided. The implications are that (1) when the swap promise is fulfilled, it's
because of some other paint, and (2) there might not be any subsequent paints,
and so we don't get a swap timestamp for FMP.

This CL fixes the problem by having the FMP detector queue the swap-promise in
when the FMP candidate is selected, which is on the Paint path. PaintTiming
reports the swap time to the FMP detector when the swap-promise is fulfilled.
The FMP detector now reports both the swap and non-swap FMP timestamps to
PaintTiming when the FMP is finally chosen.

Bug: 741961
Change-Id: I0fa9d396f6b46c90acea9f1a46e6d7cbbf5d7ef7
Reviewed-on: https://chromium-review.googlesource.com/570488Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Scott Haseley <shaseley@google.com>
Cr-Commit-Position: refs/heads/master@{#487172}

third_party/WebKit/Source/core/paint/BUILD.gn

@@ -111,6 +111,7 @@ blink_core_sources("paint") {
     "ObjectPaintProperties.h",
     "ObjectPainter.cpp",
     "ObjectPainter.h",
+    "PaintEvent.h",
     "PaintInfo.h",
     "PaintInvalidationCapableScrollableArea.cpp",
     "PaintInvalidationCapableScrollableArea.h",

third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.cpp

@@ -10,6 +10,7 @@
 #include "platform/Histogram.h"
 #include "platform/instrumentation/tracing/TraceEvent.h"
 #include "platform/loader/fetch/ResourceFetcher.h"
+#include "platform/wtf/Functional.h"
 
 namespace blink {
 
@@ -99,6 +100,9 @@ void FirstMeaningfulPaintDetector::NotifyPaint() {
   if (network2_quiet_reached_)
     return;
 
+  provisional_first_meaningful_paint_swap_ = 0.0;
+  RegisterNotifySwapTime(PaintEvent::kProvisionalFirstMeaningfulPaint);
+
   TRACE_EVENT_MARK_WITH_TIMESTAMP1(
       "loading,devtools.timeline", "firstMeaningfulPaintCandidate",
       TraceEvent::ToTraceTimestamp(provisional_first_meaningful_paint_),
@@ -181,12 +185,19 @@ void FirstMeaningfulPaintDetector::Network2QuietTimerFired(TimerBase*) {
     paint_timing_->SetFirstMeaningfulPaintCandidate(
         provisional_first_meaningful_paint_);
     // Enforce FirstContentfulPaint <= FirstMeaningfulPaint.
-    first_meaningful_paint2_quiet_ =
-        std::max(provisional_first_meaningful_paint_,
-                 paint_timing_->FirstContentfulPaint());
+    if (provisional_first_meaningful_paint_ <
+        paint_timing_->FirstContentfulPaint()) {
+      first_meaningful_paint2_quiet_ = paint_timing_->FirstContentfulPaint();
+      first_meaningful_paint2_quiet_swap_ =
+          paint_timing_->FirstContentfulPaintSwap();
+    } else {
+      first_meaningful_paint2_quiet_ = provisional_first_meaningful_paint_;
+      first_meaningful_paint2_quiet_swap_ =
+          provisional_first_meaningful_paint_swap_;
+    }
     // Report FirstMeaningfulPaint when the page reached network 2-quiet.
     paint_timing_->SetFirstMeaningfulPaint(
-        first_meaningful_paint2_quiet_,
+        first_meaningful_paint2_quiet_, first_meaningful_paint2_quiet_swap_,
         had_user_input_before_provisional_first_meaningful_paint_);
   }
   ReportHistograms();
@@ -235,6 +246,33 @@ void FirstMeaningfulPaintDetector::ReportHistograms() {
   }
 }
 
+void FirstMeaningfulPaintDetector::RegisterNotifySwapTime(PaintEvent event) {
+  paint_timing_->RegisterNotifySwapTime(
+      event, WTF::Bind(&FirstMeaningfulPaintDetector::ReportSwapTime,
+                       WrapCrossThreadWeakPersistent(this), event));
+}
+
+void FirstMeaningfulPaintDetector::ReportSwapTime(PaintEvent event,
+                                                  bool did_swap,
+                                                  double timestamp) {
+  // TODO(shaseley): Add UMAs here to see how often either of the following
+  // happen. In the first case, the FMP will be 0.0 if this is the provisional
+  // timestamp we end up using. In the second case, a swap timestamp of 0.0 is
+  // reported to PaintTiming because the |network2_quiet_timer_| already fired.
+  if (!did_swap)
+    return;
+  if (network2_quiet_reached_)
+    return;
+
+  switch (event) {
+    case PaintEvent::kProvisionalFirstMeaningfulPaint:
+      provisional_first_meaningful_paint_swap_ = timestamp;
+      return;
+    default:
+      NOTREACHED();
+  }
+}
+
 DEFINE_TRACE(FirstMeaningfulPaintDetector) {
   visitor->Trace(paint_timing_);
 }

third_party/WebKit/Source/core/paint/FirstMeaningfulPaintDetector.h

@@ -6,6 +6,7 @@
 #define FirstMeaningfulPaintDetector_h
 
 #include "core/CoreExport.h"
+#include "core/paint/PaintEvent.h"
 #include "platform/Timer.h"
 #include "platform/heap/Handle.h"
 #include "platform/wtf/Noncopyable.h"
@@ -48,6 +49,7 @@ class CORE_EXPORT FirstMeaningfulPaintDetector
   void NotifyInputEvent();
   void NotifyPaint();
   void CheckNetworkStable();
+  void ReportSwapTime(PaintEvent, bool did_swap, double timestamp);
 
   DECLARE_TRACE();
 
@@ -67,6 +69,7 @@ class CORE_EXPORT FirstMeaningfulPaintDetector
   void Network0QuietTimerFired(TimerBase*);
   void Network2QuietTimerFired(TimerBase*);
   void ReportHistograms();
+  void RegisterNotifySwapTime(PaintEvent);
 
   bool next_paint_is_meaningful_ = false;
   HadUserInput had_user_input_ = kNoUserInput;
@@ -75,6 +78,7 @@ class CORE_EXPORT FirstMeaningfulPaintDetector
 
   Member<PaintTiming> paint_timing_;
   double provisional_first_meaningful_paint_ = 0.0;
+  double provisional_first_meaningful_paint_swap_ = 0.0;
   double max_significance_so_far_ = 0.0;
   double accumulated_significance_while_having_blank_text_ = 0.0;
   unsigned prev_layout_object_count_ = 0;
@@ -83,6 +87,7 @@ class CORE_EXPORT FirstMeaningfulPaintDetector
   bool network2_quiet_reached_ = false;
   double first_meaningful_paint0_quiet_ = 0.0;
   double first_meaningful_paint2_quiet_ = 0.0;
+  double first_meaningful_paint2_quiet_swap_ = 0.0;
   TaskRunnerTimer<FirstMeaningfulPaintDetector> network0_quiet_timer_;
   TaskRunnerTimer<FirstMeaningfulPaintDetector> network2_quiet_timer_;
 };

third_party/WebKit/Source/core/paint/PaintEvent.h

+// Copyright 2017 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.
+
+#ifndef PaintEvent_h
+#define PaintEvent_h
+
+namespace blink {
+
+// Paint events that either PaintTiming or FirstMeaningfulPaintDetector receive
+// GPU swap times for.
+enum class PaintEvent {
+  kFirstPaint,
+  kFirstContentfulPaint,
+  kProvisionalFirstMeaningfulPaint,
+};
+
+}  // namespace blink
+
+#endif

third_party/WebKit/Source/core/paint/PaintTiming.cpp

@@ -4,6 +4,9 @@
 
 #include "core/paint/PaintTiming.h"
 
+#include <memory>
+#include <utility>
+
 #include "core/dom/Document.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/LocalFrame.h"
@@ -99,6 +102,7 @@ void PaintTiming::SetFirstMeaningfulPaintCandidate(double timestamp) {
 
 void PaintTiming::SetFirstMeaningfulPaint(
     double stamp,
+    double swap_stamp,
     FirstMeaningfulPaintDetector::HadUserInput had_input) {
   DCHECK_EQ(first_meaningful_paint_, 0.0);
   TRACE_EVENT_MARK_WITH_TIMESTAMP2("loading,rail,devtools.timeline",
@@ -110,8 +114,8 @@ void PaintTiming::SetFirstMeaningfulPaint(
   // changes caused by user interactions wouldn't be considered as FMP.
   if (had_input == FirstMeaningfulPaintDetector::kNoUserInput) {
     first_meaningful_paint_ = stamp;
+    first_meaningful_paint_swap_ = swap_stamp;
     NotifyPaintTimingChanged();
-    RegisterNotifySwapTime(PaintEvent::kFirstMeaningfulPaint);
   }
 
   ReportUserInputHistogram(had_input);
@@ -179,6 +183,14 @@ void PaintTiming::SetFirstContentfulPaint(double stamp) {
 }
 
 void PaintTiming::RegisterNotifySwapTime(PaintEvent event) {
+  RegisterNotifySwapTime(event,
+                         WTF::Bind(&PaintTiming::ReportSwapTime,
+                                   WrapCrossThreadWeakPersistent(this), event));
+}
+
+void PaintTiming::RegisterNotifySwapTime(
+    PaintEvent event,
+    std::unique_ptr<WTF::Function<void(bool, double)>> callback) {
   // ReportSwapTime on layerTreeView will queue a swap-promise, the callback is
   // called when the swap for current render frame completes or fails to happen.
   if (!GetFrame() || !GetFrame()->GetPage())
@@ -186,9 +198,7 @@ void PaintTiming::RegisterNotifySwapTime(PaintEvent event) {
   if (WebLayerTreeView* layerTreeView =
           GetFrame()->GetPage()->GetChromeClient().GetWebLayerTreeView(
               GetFrame())) {
-    layerTreeView->NotifySwapTime(ConvertToBaseCallback(
-        WTF::Bind(&PaintTiming::ReportSwapTime,
-                  WrapCrossThreadWeakPersistent(this), event)));
+    layerTreeView->NotifySwapTime(ConvertToBaseCallback(std::move(callback)));
   }
 }
 
@@ -209,11 +219,9 @@ void PaintTiming::ReportSwapTime(PaintEvent event,
       if (performance)
         performance->AddFirstContentfulPaintTiming(first_contentful_paint_);
       return;
-    case PaintEvent::kFirstMeaningfulPaint:
-      first_meaningful_paint_swap_ = timestamp;
-      return;
+    default:
+      NOTREACHED();
   }
-  NOTREACHED();
 }
 
 }  // namespace blink

third_party/WebKit/Source/core/paint/PaintTiming.h

@@ -5,10 +5,14 @@
 #ifndef PaintTiming_h
 #define PaintTiming_h
 
+#include <memory>
+
 #include "core/dom/Document.h"
 #include "core/paint/FirstMeaningfulPaintDetector.h"
+#include "core/paint/PaintEvent.h"
 #include "platform/Supplementable.h"
 #include "platform/heap/Handle.h"
+#include "platform/wtf/Functional.h"
 #include "platform/wtf/Noncopyable.h"
 
 namespace blink {
@@ -26,12 +30,6 @@ class CORE_EXPORT PaintTiming final
  public:
   virtual ~PaintTiming() {}
 
-  enum class PaintEvent {
-    kFirstPaint,
-    kFirstContentfulPaint,
-    kFirstMeaningfulPaint
-  };
-
   static PaintTiming& From(Document&);
 
   // mark*() methods record the time for the given paint event, record a trace
@@ -52,6 +50,7 @@ class CORE_EXPORT PaintTiming final
   void SetFirstMeaningfulPaintCandidate(double timestamp);
   void SetFirstMeaningfulPaint(
       double stamp,
+      double swap_stamp,
       FirstMeaningfulPaintDetector::HadUserInput had_input);
   void NotifyPaint(bool is_first_paint, bool text_painted, bool image_painted);
 
@@ -67,6 +66,9 @@ class CORE_EXPORT PaintTiming final
   // painted. For instance, the first time that text or image content was
   // painted.
   double FirstContentfulPaint() const { return first_contentful_paint_; }
+  double FirstContentfulPaintSwap() const {
+    return first_contentful_paint_swap_;
+  }
 
   // firstTextPaint returns the first time that text content was painted.
   double FirstTextPaint() const { return first_text_paint_; }
@@ -90,6 +92,9 @@ class CORE_EXPORT PaintTiming final
     return *fmp_detector_;
   }
 
+  void RegisterNotifySwapTime(
+      PaintEvent,
+      std::unique_ptr<WTF::Function<void(bool, double)>> callback);
   void ReportSwapTime(PaintEvent, bool did_swap, double timestamp);
 
   DECLARE_VIRTUAL_TRACE();