Save EventMetrics to report after processing is done

Currently, as soon as we notice a frame needs to be produced while
processing an event, a copy of its EventMetrics is saved for reporting.
The plan is to add the timestamp of when the processing of the event is
done (on both compositor and main threads) to this object to be included
in reports. To allow this, the copy of the object should be saved after
the processing is done and the timestamp is added. This CL changes
EventsMetricsManager to get a callback for its scoped monitor instead of
a pointer to the metrics object. The callback will be called when the
scoped monitor is destroyed (i.e. when the event processing is
finished). The client code can update metrics object as appropriate and
return it to EventsMetricsManager if necessary. Also, if the metrics
object is supposed to be saved, the client code can safely move the
object to EventsMetricsManager as it won't need it anymore.

Bug: 1054009
Change-Id: I503bf64171f08f8233d7f738304b0e2e3a96da31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2488549
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Mohsen Izadi <mohsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823583}

cc/input/input_handler.h

@@ -35,7 +35,6 @@ class LatencyInfo;
 
 namespace cc {
 
-class EventMetrics;
 class CompositorDelegateForInput;
 class ScrollElasticityHelper;
 
@@ -328,13 +327,18 @@ class CC_EXPORT InputHandler {
   virtual std::unique_ptr<SwapPromiseMonitor>
   CreateLatencyInfoSwapPromiseMonitor(ui::LatencyInfo* latency) = 0;
 
-  // During the lifetime of the returned EventsMetricsManager::ScopedMonitor, if
-  // SetNeedsOneBeginImplFrame() or SetNeedsRedraw() are called on
-  // LayerTreeHostImpl or a scroll animation is updated, |event_metrics| will be
-  // saved for reporting event latency metrics. It is allowed to pass nullptr as
-  // |event_metrics| in which case the return value would also be nullptr.
+  // Returns a new instance of `EventsMetricsManager::ScopedMonitor` to monitor
+  // the scope of handling an event. If `done_callback` is not a null callback,
+  // it will be called when the scope ends. If During the lifetime of the scoped
+  // monitor, `SetNeedsOneBeginImplFrame()` or `SetNeedsRedraw()` are called on
+  // `LayerTreeHostImpl` or a scroll animation is updated, the callback will be
+  // called in the end with `handled` argument set to true, denoting that the
+  // event was handled and the client should return `EventMetrics` associated
+  // with the event if it is interested in reporting event latency metrics for
+  // it.
   virtual std::unique_ptr<EventsMetricsManager::ScopedMonitor>
-  GetScopedEventMetricsMonitor(const EventMetrics* event_metrics) = 0;
+  GetScopedEventMetricsMonitor(
+      EventsMetricsManager::ScopedMonitor::DoneCallback done_callback) = 0;
 
   virtual ScrollElasticityHelper* CreateScrollElasticityHelper() = 0;
 

cc/input/threaded_input_handler.cc

@@ -801,9 +801,9 @@ ThreadedInputHandler::CreateLatencyInfoSwapPromiseMonitor(
 
 std::unique_ptr<EventsMetricsManager::ScopedMonitor>
 ThreadedInputHandler::GetScopedEventMetricsMonitor(
-    const EventMetrics* event_metrics) {
+    EventsMetricsManager::ScopedMonitor::DoneCallback done_callback) {
   return compositor_delegate_.GetImplDeprecated().GetScopedEventMetricsMonitor(
-      event_metrics);
+      std::move(done_callback));
 }
 
 ScrollElasticityHelper* ThreadedInputHandler::CreateScrollElasticityHelper() {

cc/input/threaded_input_handler.h

@@ -87,7 +87,8 @@ class CC_EXPORT ThreadedInputHandler : public InputHandler,
   std::unique_ptr<SwapPromiseMonitor> CreateLatencyInfoSwapPromiseMonitor(
       ui::LatencyInfo* latency) override;
   std::unique_ptr<EventsMetricsManager::ScopedMonitor>
-  GetScopedEventMetricsMonitor(const EventMetrics* event_metrics) override;
+  GetScopedEventMetricsMonitor(
+      EventsMetricsManager::ScopedMonitor::DoneCallback done_callback) override;
   ScrollElasticityHelper* CreateScrollElasticityHelper() override;
   bool GetScrollOffsetForLayer(ElementId element_id,
                                gfx::ScrollOffset* offset) override;

cc/metrics/events_metrics_manager.cc

@@ -12,49 +12,55 @@
 #include "base/stl_util.h"
 
 namespace cc {
-namespace {
 
-// A `ScopedMonitor` implementation that takes a callback and runs it upon
-// destruction.
-class ScopedMonitorImpl : public EventsMetricsManager::ScopedMonitor {
+class EventsMetricsManager::ScopedMonitorImpl
+    : public EventsMetricsManager::ScopedMonitor {
  public:
-  explicit ScopedMonitorImpl(base::OnceClosure done_callback)
-      : done_callback_runner_(std::move(done_callback)) {}
+  ScopedMonitorImpl(EventsMetricsManager* manager, DoneCallback done_callback)
+      : manager_(manager), done_callback_(std::move(done_callback)) {
+    DCHECK_NE(manager, nullptr);
+  }
+
+  ~ScopedMonitorImpl() override {
+    std::unique_ptr<EventMetrics> metrics;
+    if (!done_callback_.is_null()) {
+      const bool handled = save_metrics_;
+      metrics = std::move(done_callback_).Run(handled);
+
+      // If `handled` is false, the callback should return nullptr.
+      DCHECK(handled || !metrics);
+    }
+    manager_->OnScopedMonitorEnded(std::move(metrics));
+  }
+
+  void set_save_metrics() { save_metrics_ = true; }
 
  private:
-  // Holds a callback closure that is run automatically when the scoped monitor
-  // is destroyed.
-  base::ScopedClosureRunner done_callback_runner_;
+  EventsMetricsManager* const manager_;
+  DoneCallback done_callback_;
+  bool save_metrics_ = false;
 };
 
-}  // namespace
-
 EventsMetricsManager::ScopedMonitor::~ScopedMonitor() = default;
 
 EventsMetricsManager::EventsMetricsManager() = default;
 EventsMetricsManager::~EventsMetricsManager() = default;
 
 std::unique_ptr<EventsMetricsManager::ScopedMonitor>
-EventsMetricsManager::GetScopedMonitor(const EventMetrics* event_metrics) {
-  active_events_.push_back(event_metrics);
-  return std::make_unique<ScopedMonitorImpl>(base::BindOnce(
-      &EventsMetricsManager::OnScopedMonitorEnded, weak_factory_.GetWeakPtr()));
+EventsMetricsManager::GetScopedMonitor(
+    ScopedMonitor::DoneCallback done_callback) {
+  auto monitor =
+      std::make_unique<ScopedMonitorImpl>(this, std::move(done_callback));
+  active_scoped_monitors_.push_back(monitor.get());
+  return monitor;
 }
 
 void EventsMetricsManager::SaveActiveEventMetrics() {
-  if (active_events_.size() > 0 && active_events_.back()) {
-    // TODO(crbug.com/1054009): It is fine to make a copy of active EventMetrics
-    // object here as we are not going to change it later. However, the plan is
-    // to add timestamp of when the processing is done to this object. Since end
-    // of the processing happens after this code, we can't simply make a copy
-    // here. In that case, here we can just mark the event for saving and do the
-    // actual copy when the scoped monitor is destroyed which happens after the
-    // event processing is done.
-    saved_events_.push_back(*active_events_.back());
-
-    // The items will be popped from the stack when the scoped monitor is ended,
-    // but replace it nullptr here to avoid reporting it multiple times.
-    active_events_.back() = nullptr;
+  if (active_scoped_monitors_.size() > 0) {
+    // Here we just set the flag to save the active metrics. The actual saving
+    // happens when the scoped monitor is destroyed to give clients opportunity
+    // to use/update the metrics object until the end of their processing.
+    active_scoped_monitors_.back()->set_save_metrics();
   }
 }
 
@@ -64,9 +70,13 @@ std::vector<EventMetrics> EventsMetricsManager::TakeSavedEventsMetrics() {
   return result;
 }
 
-void EventsMetricsManager::OnScopedMonitorEnded() {
-  DCHECK_GT(active_events_.size(), 0u);
-  active_events_.pop_back();
+void EventsMetricsManager::OnScopedMonitorEnded(
+    std::unique_ptr<EventMetrics> metrics) {
+  DCHECK_GT(active_scoped_monitors_.size(), 0u);
+  active_scoped_monitors_.pop_back();
+
+  if (metrics)
+    saved_events_.push_back(*metrics);
 }
 
 }  // namespace cc

cc/metrics/events_metrics_manager.h

@@ -8,7 +8,7 @@
 #include <memory>
 #include <vector>
 
-#include "base/memory/weak_ptr.h"
+#include "base/callback.h"
 #include "base/time/time.h"
 #include "cc/cc_export.h"
 #include "cc/metrics/event_metrics.h"
@@ -28,6 +28,15 @@ class CC_EXPORT EventsMetricsManager {
   // nested.
   class ScopedMonitor {
    public:
+    // Type of the callback function that is called after a scoped monitor goes
+    // out of scope. `handled` specifies whether the corresponding event was
+    // handled in which case the function should return its corresponding
+    // `EventMetrics` object for reporting purposes. Since calling this callback
+    // denotes the end of event processing, clients can use this to set relevant
+    // timestamps to the metrics object, before potentially returning it.
+    using DoneCallback =
+        base::OnceCallback<std::unique_ptr<EventMetrics>(bool handled)>;
+
     ScopedMonitor() = default;
     virtual ~ScopedMonitor() = 0;
 
@@ -42,14 +51,16 @@ class CC_EXPORT EventsMetricsManager {
   EventsMetricsManager& operator=(const EventsMetricsManager&) = delete;
 
   // Called by clients when they start handling an event. Destruction of the
-  // scoped monitor indicates the end of event handling. |event_metrics| is
-  // allowed to be nullptr, which means the client is not interested in
-  // reporting metrics for this specific events.
+  // scoped monitor indicates the end of event handling which ends in calling
+  // `done_callback`. `done_callback` is allowed to be a null callback, which
+  // means the client is not interested in reporting metrics for the event being
+  // handled in this specific scope.
   std::unique_ptr<ScopedMonitor> GetScopedMonitor(
-      const EventMetrics* event_metrics);
+      ScopedMonitor::DoneCallback done_callback);
 
   // Called by clients when a frame needs to be produced. If any scoped monitor
-  // is active at this time, its corresponding event metrics would be saved.
+  // is active at this time, its corresponding event metrics would be marked to
+  // be saved when it goes out of scope.
   void SaveActiveEventMetrics();
 
   // Empties the list of saved EventMetrics objects, returning them to the
@@ -61,15 +72,17 @@ class CC_EXPORT EventsMetricsManager {
   }
 
  private:
-  void OnScopedMonitorEnded();
+  class ScopedMonitorImpl;
 
-  // Stack of active, potentially nested, event metrics
-  std::vector<const EventMetrics*> active_events_;
+  // Called when the most nested scoped monitor is destroyed. If the monitored
+  // metrics need to be saved it will be passed in as `metrics`.
+  void OnScopedMonitorEnded(std::unique_ptr<EventMetrics> metrics);
 
-  // List of saved event metrics.
-  std::vector<EventMetrics> saved_events_;
+  // Stack of active, potentially nested, scoped monitors.
+  std::vector<ScopedMonitorImpl*> active_scoped_monitors_;
 
-  base::WeakPtrFactory<EventsMetricsManager> weak_factory_{this};
+  // List of event metrics saved for reporting.
+  std::vector<EventMetrics> saved_events_;
 };
 
 }  // namespace cc

cc/metrics/events_metrics_manager_unittest.cc

@@ -7,6 +7,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/stl_util.h"
 #include "cc/metrics/event_metrics.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -15,6 +16,24 @@
 #include "ui/events/types/scroll_input_type.h"
 
 namespace cc {
+namespace {
+
+std::unique_ptr<EventMetrics> CloneMetrics(const EventMetrics& metrics) {
+  return std::make_unique<EventMetrics>(metrics);
+}
+
+EventsMetricsManager::ScopedMonitor::DoneCallback CreateSimpleDoneCallback(
+    std::unique_ptr<EventMetrics> metrics) {
+  return base::BindOnce(
+      [](std::unique_ptr<EventMetrics> metrics, bool handled) {
+        std::unique_ptr<EventMetrics> result =
+            handled ? std::move(metrics) : nullptr;
+        return result;
+      },
+      std::move(metrics));
+}
+
+}  // namespace
 
 #define EXPECT_SCOPED(statements) \
   {                               \
@@ -93,7 +112,8 @@ TEST_F(EventsMetricsManagerTest, EventsMetricsSaved) {
 
   for (auto& event : events) {
     {
-      auto monitor = manager_.GetScopedMonitor(event.first.get());
+      auto monitor = manager_.GetScopedMonitor(
+          CreateSimpleDoneCallback(std::move(event.first)));
       if (event.second == Behavior::kSaveInsideScope)
         manager_.SaveActiveEventMetrics();
       // Ending the scope destroys the |monitor|.
@@ -124,13 +144,13 @@ TEST_F(EventsMetricsManagerTest, NestedEventsMetrics) {
 
   struct {
     // Metrics to use for the outer scope.
-    const EventMetrics* outer_metrics;
+    std::unique_ptr<EventMetrics> outer_metrics;
 
     // Whether to save the outer scope metrics before starting the inner scope.
     bool save_outer_metrics_before_inner;
 
     // Metrics to use for the inner scope.
-    const EventMetrics* inner_metrics;
+    std::unique_ptr<EventMetrics> inner_metrics;
 
     // Whether to save the inner scope metrics.
     bool save_inner_metrics;
@@ -143,37 +163,37 @@ TEST_F(EventsMetricsManagerTest, NestedEventsMetrics) {
   } configs[] = {
       // Config #0.
       {
-          /*outer_metrics=*/events[0].get(),
+          /*outer_metrics=*/CloneMetrics(*events[0]),
           /*save_outer_metrics_before_inner=*/true,
-          /*inner_metrics=*/events[1].get(),
+          /*inner_metrics=*/CloneMetrics(*events[1]),
           /*save_inner_metrics=*/true,
           /*save_outer_metrics_after_inner=*/false,
           /*expected_saved_metrics=*/{*events[0], *events[1]},
       },
 
       // Config #1.
       {
-          /*outer_metrics=*/events[0].get(),
+          /*outer_metrics=*/CloneMetrics(*events[0]),
           /*save_outer_metrics_before_inner=*/false,
-          /*inner_metrics=*/events[1].get(),
+          /*inner_metrics=*/CloneMetrics(*events[1]),
           /*save_inner_metrics=*/true,
           /*save_outer_metrics_after_inner=*/true,
           /*expected_saved_metrics=*/{*events[0], *events[1]},
       },
 
       // Config #2.
       {
-          /*outer_metrics=*/events[0].get(),
+          /*outer_metrics=*/CloneMetrics(*events[0]),
           /*save_outer_metrics_before_inner=*/true,
-          /*inner_metrics=*/events[1].get(),
+          /*inner_metrics=*/CloneMetrics(*events[1]),
           /*save_inner_metrics=*/true,
           /*save_outer_metrics_after_inner=*/true,
           /*expected_saved_metrics=*/{*events[0], *events[1]},
       },
 
       // Config #3.
       {
-          /*outer_metrics=*/events[0].get(),
+          /*outer_metrics=*/CloneMetrics(*events[0]),
           /*save_outer_metrics_before_inner=*/false,
           /*inner_metrics=*/nullptr,
           /*save_inner_metrics=*/false,
@@ -185,7 +205,7 @@ TEST_F(EventsMetricsManagerTest, NestedEventsMetrics) {
       {
           /*outer_metrics=*/nullptr,
           /*save_outer_metrics_before_inner=*/false,
-          /*inner_metrics=*/events[0].get(),
+          /*inner_metrics=*/CloneMetrics(*events[0]),
           /*save_inner_metrics=*/true,
           /*save_outer_metrics_after_inner=*/false,
           /*expected_saved_metrics=*/{*events[0]},
@@ -195,11 +215,13 @@ TEST_F(EventsMetricsManagerTest, NestedEventsMetrics) {
   for (size_t i = 0; i < base::size(configs); i++) {
     auto& config = configs[i];
     {
-      auto outer_monitor = manager_.GetScopedMonitor(config.outer_metrics);
+      auto outer_monitor = manager_.GetScopedMonitor(
+          CreateSimpleDoneCallback(std::move(config.outer_metrics)));
       if (config.save_outer_metrics_before_inner)
         manager_.SaveActiveEventMetrics();
       {
-        auto inner_monitor = manager_.GetScopedMonitor(config.inner_metrics);
+        auto inner_monitor = manager_.GetScopedMonitor(
+            CreateSimpleDoneCallback(std::move(config.inner_metrics)));
         if (config.save_inner_metrics)
           manager_.SaveActiveEventMetrics();
       }

cc/trees/layer_tree_host.cc

@@ -254,8 +254,9 @@ SwapPromiseManager* LayerTreeHost::GetSwapPromiseManager() {
 }
 
 std::unique_ptr<EventsMetricsManager::ScopedMonitor>
-LayerTreeHost::GetScopedEventMetricsMonitor(const EventMetrics* event_metrics) {
-  return events_metrics_manager_.GetScopedMonitor(event_metrics);
+LayerTreeHost::GetScopedEventMetricsMonitor(
+    EventsMetricsManager::ScopedMonitor::DoneCallback done_callback) {
+  return events_metrics_manager_.GetScopedMonitor(std::move(done_callback));
 }
 
 void LayerTreeHost::ClearEventsMetrics() {

cc/trees/layer_tree_host.h

@@ -37,7 +37,6 @@
 #include "cc/layers/layer_collections.h"
 #include "cc/layers/layer_list_iterator.h"
 #include "cc/metrics/begin_main_frame_metrics.h"
-#include "cc/metrics/event_metrics.h"
 #include "cc/metrics/events_metrics_manager.h"
 #include "cc/metrics/frame_sequence_tracker.h"
 #include "cc/paint/node_id.h"
@@ -185,7 +184,8 @@ class CC_EXPORT LayerTreeHost : public MutatorHostClient {
   SwapPromiseManager* GetSwapPromiseManager();
 
   std::unique_ptr<EventsMetricsManager::ScopedMonitor>
-  GetScopedEventMetricsMonitor(const EventMetrics* event_metrics);
+  GetScopedEventMetricsMonitor(
+      EventsMetricsManager::ScopedMonitor::DoneCallback done_callback);
   void ClearEventsMetrics();
 
   size_t saved_events_metrics_count_for_testing() const {

cc/trees/layer_tree_host_impl.cc

@@ -979,8 +979,8 @@ LayerTreeHostImpl::CreateLatencyInfoSwapPromiseMonitor(
 
 std::unique_ptr<EventsMetricsManager::ScopedMonitor>
 LayerTreeHostImpl::GetScopedEventMetricsMonitor(
-    const EventMetrics* event_metrics) {
-  return events_metrics_manager_.GetScopedMonitor(event_metrics);
+    EventsMetricsManager::ScopedMonitor::DoneCallback done_callback) {
+  return events_metrics_manager_.GetScopedMonitor(std::move(done_callback));
 }
 
 void LayerTreeHostImpl::NotifyInputEvent() {