Throttle mostly unresponsive CompositorFrameSinkClients

Unthrottling clients as soon as they acked any begin frame was not
effective in preventing a huge backlog of OnBeginFrame() IPCs from
building up. Instead of allowing any ack to unthrottle, instead count
the number of sent BeginFrameArgs and received BeginFrameAcks.

To avoid problems where clients don't properly ack all OnBeginFrame()
messages but are still responsive, if the client acks the latest
BeginFrameArgs then we treat it like they have acked all outstanding
begin frames.

Bug: 1011441
Change-Id: I9d0b73a189945d03c4e0590b1acea52df020ee03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1947186
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721467}

components/viz/service/BUILD.gn

@@ -125,6 +125,8 @@ viz_component("service") {
     "display_embedder/viz_process_context_provider.h",
     "display_embedder/vsync_parameter_listener.cc",
     "display_embedder/vsync_parameter_listener.h",
+    "frame_sinks/begin_frame_tracker.cc",
+    "frame_sinks/begin_frame_tracker.h",
     "frame_sinks/compositor_frame_sink_impl.cc",
     "frame_sinks/compositor_frame_sink_impl.h",
     "frame_sinks/compositor_frame_sink_support.cc",
@@ -438,6 +440,7 @@ viz_source_set("unit_tests") {
     "display_embedder/skia_output_surface_impl_unittest.cc",
     "display_embedder/software_output_surface_unittest.cc",
     "display_embedder/vsync_parameter_listener_unittest.cc",
+    "frame_sinks/begin_frame_tracker_unittest.cc",
     "frame_sinks/compositor_frame_sink_support_unittest.cc",
     "frame_sinks/direct_layer_tree_frame_sink_unittest.cc",
     "frame_sinks/frame_sink_manager_unittest.cc",

components/viz/service/frame_sinks/begin_frame_tracker.cc

+// Copyright 2019 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.
+
+#include "components/viz/service/frame_sinks/begin_frame_tracker.h"
+
+namespace viz {
+
+void BeginFrameTracker::SentBeginFrame(const BeginFrameArgs& args) {
+  ++outstanding_begin_frames_;
+  last_source_id_ = args.source_id;
+  last_sequence_number_ = args.sequence_number;
+}
+
+void BeginFrameTracker::ReceivedAck(const BeginFrameAck& ack) {
+  if (MatchesLastSent(ack)) {
+    // If the BeginFrameAck matches the last sent BeginFrameArgs then we know
+    // the client has read every message from the pipe. While the client
+    // should send an ack for every args, this guards against bugs that make a
+    // responsive client occasional drop a begin frame with no ack. Otherwise
+    // the occasional dropped ack would add up and eventually throttle then
+    // stop sending begin frames entirely.
+    outstanding_begin_frames_ = 0;
+  } else if (outstanding_begin_frames_ > 0) {
+    // The condition above makes it such that we aren't necessarily evenly
+    // incrementing/decrementing |outstanding_begin_frames_|, so ensure it
+    // never goes negative.
+    --outstanding_begin_frames_;
+  }
+}
+
+bool BeginFrameTracker::ShouldThrottleBeginFrame() const {
+  return outstanding_begin_frames_ >= kLimitThrottle &&
+         outstanding_begin_frames_ < kLimitStop;
+}
+
+bool BeginFrameTracker::ShouldStopBeginFrame() const {
+  return outstanding_begin_frames_ >= kLimitStop;
+}
+
+bool BeginFrameTracker::MatchesLastSent(const BeginFrameAck& ack) {
+  return last_source_id_ == ack.source_id &&
+         last_sequence_number_ == ack.sequence_number;
+}
+
+}  // namespace viz

components/viz/service/frame_sinks/begin_frame_tracker.h

+// Copyright 2019 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 COMPONENTS_VIZ_SERVICE_FRAME_SINKS_BEGIN_FRAME_TRACKER_H_
+#define COMPONENTS_VIZ_SERVICE_FRAME_SINKS_BEGIN_FRAME_TRACKER_H_
+
+#include "components/viz/common/frame_sinks/begin_frame_args.h"
+#include "components/viz/service/viz_service_export.h"
+
+namespace viz {
+
+// Keeps track of OnBeginFrame() messages sent the client and acks received back
+// from the client in order to throttle unresponsive clients. This is to prevent
+// sending a large number of IPCs to a client that is unresponsive and having
+// the message queue balloon in size. Tolerates clients occasionally dropping
+// an OnBeginFrame() message and not acking as long as they keep responding to
+// the latest BeginFrameArgs.
+class VIZ_SERVICE_EXPORT BeginFrameTracker {
+ public:
+  // Defines the number of begin frames that have been sent to a client without
+  // a response before we throttle or stop sending begin frames altogether.
+  static constexpr int kLimitStop = 100;
+  static constexpr int kLimitThrottle = 10;
+
+  // To be called every time OnBeginFrame() is sent.
+  void SentBeginFrame(const BeginFrameArgs& args);
+
+  // To be called every time a BeginFrameAck is received back from the client.
+  void ReceivedAck(const BeginFrameAck& ack);
+
+  bool ShouldThrottleBeginFrame() const;
+  bool ShouldStopBeginFrame() const;
+
+ private:
+  bool MatchesLastSent(const BeginFrameAck& ack);
+
+  int outstanding_begin_frames_ = 0;
+
+  uint64_t last_source_id_ = 0;
+  uint64_t last_sequence_number_ = BeginFrameArgs::kInvalidFrameNumber;
+};
+
+}  // namespace viz
+
+#endif  // COMPONENTS_VIZ_SERVICE_FRAME_SINKS_BEGIN_FRAME_TRACKER_H_

components/viz/service/frame_sinks/begin_frame_tracker_unittest.cc

+// Copyright 2019 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.
+
+#include "components/viz/service/frame_sinks/begin_frame_tracker.h"
+
+#include <queue>
+
+#include "base/containers/queue.h"
+#include "components/viz/test/begin_frame_args_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace viz {
+namespace {
+
+class BeginFrameTrackerTest : public testing::Test {
+ public:
+  void SendNextBeginFrame() {
+    BeginFrameArgs args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE,
+                                                         0, sequence_number_++);
+    pending_acks_.push(BeginFrameAck(args, true));
+    tracker_.SentBeginFrame(args);
+  }
+
+  void SendAck() {
+    tracker_.ReceivedAck(pending_acks_.front());
+    pending_acks_.pop();
+  }
+
+  void DropAck() { pending_acks_.pop(); }
+
+ protected:
+  BeginFrameTracker tracker_;
+  uint64_t sequence_number_ = 1;
+  std::queue<BeginFrameAck> pending_acks_;
+};
+
+// Verify that BeginFrameTracker throttles and unthrottles correctly.
+TEST_F(BeginFrameTrackerTest, Throttle) {
+  for (int i = 0; i < BeginFrameTracker::kLimitThrottle; ++i) {
+    EXPECT_FALSE(tracker_.ShouldThrottleBeginFrame());
+    EXPECT_FALSE(tracker_.ShouldStopBeginFrame());
+    SendNextBeginFrame();
+  }
+
+  EXPECT_TRUE(tracker_.ShouldThrottleBeginFrame());
+  EXPECT_FALSE(tracker_.ShouldStopBeginFrame());
+
+  SendAck();
+
+  EXPECT_FALSE(tracker_.ShouldThrottleBeginFrame());
+  EXPECT_FALSE(tracker_.ShouldStopBeginFrame());
+}
+
+// Verify that BeginFrameTracker stops sending begin frames after kLimitStop.
+TEST_F(BeginFrameTrackerTest, Stop) {
+  for (int i = 0; i < BeginFrameTracker::kLimitStop; ++i) {
+    EXPECT_FALSE(tracker_.ShouldStopBeginFrame());
+    SendNextBeginFrame();
+  }
+
+  EXPECT_FALSE(tracker_.ShouldThrottleBeginFrame());
+  EXPECT_TRUE(tracker_.ShouldStopBeginFrame());
+
+  SendAck();
+
+  EXPECT_TRUE(tracker_.ShouldThrottleBeginFrame());
+  EXPECT_FALSE(tracker_.ShouldStopBeginFrame());
+}
+
+// Verify that BeginFrameTracker doesn't throttle a client that only acks half
+// the time, as long as they ack the latest BeginFrameArgs.
+TEST_F(BeginFrameTrackerTest, AllowDroppedAcks) {
+  for (int i = 0; i < BeginFrameTracker::kLimitThrottle * 4; ++i) {
+    EXPECT_FALSE(tracker_.ShouldThrottleBeginFrame());
+    EXPECT_FALSE(tracker_.ShouldStopBeginFrame());
+    SendNextBeginFrame();
+
+    if (i % 2)
+      SendAck();
+    else
+      DropAck();
+  }
+}
+
+}  // namespace
+}  // namespace viz

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -312,10 +312,6 @@ void CompositorFrameSinkSupport::EvictLastActiveSurface() {
 }
 
 void CompositorFrameSinkSupport::SetNeedsBeginFrame(bool needs_begin_frame) {
-  // Reset outstanding begin frames. This isn't a response to the begin frame
-  // directly but at least we know the client is responsive.
-  outstanding_begin_frames_ = 0;
-
   client_needs_begin_frame_ = needs_begin_frame;
   UpdateNeedsBeginFramesInternal();
 }
@@ -338,7 +334,7 @@ void CompositorFrameSinkSupport::DidNotProduceFrame(const BeginFrameAck& ack) {
                ack.sequence_number);
   DCHECK_GE(ack.sequence_number, BeginFrameArgs::kStartingFrameNumber);
 
-  outstanding_begin_frames_ = 0;
+  begin_frame_tracker_.ReceivedAck(ack);
 
   // Override the has_damage flag (ignoring invalid data from clients).
   BeginFrameAck modified_ack(ack);
@@ -672,7 +668,7 @@ void CompositorFrameSinkSupport::OnBeginFrame(const BeginFrameArgs& args) {
                            "IssueBeginFrame");
     last_frame_time_ = args.frame_time;
     client_->OnBeginFrame(copy_args, std::move(frame_timing_details_));
-    ++outstanding_begin_frames_;
+    begin_frame_tracker_.SentBeginFrame(args);
     frame_sink_manager_->DidBeginFrame(frame_sink_id_, args);
     frame_timing_details_.clear();
     UpdateNeedsBeginFramesInternal();
@@ -716,7 +712,7 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrame(
     base::Optional<HitTestRegionList> hit_test_region_list,
     uint64_t submit_time,
     mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback callback) {
-  outstanding_begin_frames_ = 0;
+  begin_frame_tracker_.ReceivedAck(frame.metadata.begin_frame_ack);
 
   SubmitResult result = MaybeSubmitCompositorFrameInternal(
       local_surface_id, std::move(frame), std::move(hit_test_region_list),
@@ -828,7 +824,7 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
   }
 
   // Stop sending BeginFrames to clients that are totally unresponsive.
-  if (outstanding_begin_frames_ >= kOutstandingFramesStop) {
+  if (begin_frame_tracker_.ShouldStopBeginFrame()) {
     RecordShouldSendBeginFrame(SendBeginFrameResult::kStopUnresponsiveClient);
     return false;
   }
@@ -839,7 +835,7 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
       (frame_time - last_frame_time_) < base::TimeDelta::FromSeconds(1);
 
   // Throttle clients that are unresponsive.
-  if (can_throttle && outstanding_begin_frames_ >= kOutstandingFramesThrottle) {
+  if (can_throttle && begin_frame_tracker_.ShouldThrottleBeginFrame()) {
     RecordShouldSendBeginFrame(
         SendBeginFrameResult::kThrottleUnresponsiveClient);
     return false;

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -19,6 +19,7 @@
 #include "components/viz/common/quads/compositor_frame.h"
 #include "components/viz/common/surfaces/surface_info.h"
 #include "components/viz/common/surfaces/surface_range.h"
+#include "components/viz/service/frame_sinks/begin_frame_tracker.h"
 #include "components/viz/service/frame_sinks/surface_resource_holder.h"
 #include "components/viz/service/frame_sinks/surface_resource_holder_client.h"
 #include "components/viz/service/frame_sinks/video_capture/capturable_frame_sink.h"
@@ -69,11 +70,6 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   // us to have one outstanding undrawn frame under normal operation.
   static constexpr uint32_t kUndrawnFrameLimit = 3;
 
-  // Defines the number of begin frames that have been sent to a client without
-  // a response before we throttle or stop sending begin frames altogether.
-  static constexpr int kOutstandingFramesStop = 100;
-  static constexpr int kOutstandingFramesThrottle = 10;
-
   CompositorFrameSinkSupport(mojom::CompositorFrameSinkClient* client,
                              FrameSinkManagerImpl* frame_sink_manager,
                              const FrameSinkId& frame_sink_id,
@@ -333,11 +329,7 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   bool callback_received_receive_ack_ = true;
   uint32_t trace_sequence_ = 0;
 
-  // Keep track of the number of OnBeginFrame() messages sent the client without
-  // getting a response back. This is to prevent sending a large number of IPCs
-  // to a client that is unresponsive and having the message queue balloon in
-  // size.
-  int outstanding_begin_frames_ = 0;
+  BeginFrameTracker begin_frame_tracker_;
 
   // Maps |frame_token| to the timestamp when that frame was received. This
   // timestamp is combined with the information received in OnSurfacePresented()

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -1349,8 +1349,7 @@ TEST_F(CompositorFrameSinkSupportTest, ThrottleUnresponsiveClient) {
 
   // Issue ten OnBeginFrame() messages with no response. They should all be
   // received by the client.
-  for (; sent_frames < CompositorFrameSinkSupport::kOutstandingFramesThrottle;
-       ++sent_frames) {
+  for (; sent_frames < BeginFrameTracker::kLimitThrottle; ++sent_frames) {
     frametime += interval;
 
     args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
@@ -1360,8 +1359,7 @@ TEST_F(CompositorFrameSinkSupportTest, ThrottleUnresponsiveClient) {
     testing::Mock::VerifyAndClearExpectations(&mock_client);
   }
 
-  for (; sent_frames < CompositorFrameSinkSupport::kOutstandingFramesStop;
-       ++sent_frames) {
+  for (; sent_frames < BeginFrameTracker::kLimitStop; ++sent_frames) {
     base::TimeTicks unthrottle_time =
         frametime + base::TimeDelta::FromSeconds(1);
 
@@ -1390,6 +1388,8 @@ TEST_F(CompositorFrameSinkSupportTest, ThrottleUnresponsiveClient) {
     testing::Mock::VerifyAndClearExpectations(&mock_client);
   }
 
+  BeginFrameArgs last_sent_args = args;
+
   // The client should no longer receive OnBeginFrame() until it becomes
   // responsive again.
   frametime += base::TimeDelta::FromMinutes(1);
@@ -1401,7 +1401,7 @@ TEST_F(CompositorFrameSinkSupportTest, ThrottleUnresponsiveClient) {
 
   // The client becomes responsive again. The next OnBeginFrame() message should
   // be delivered.
-  support->DidNotProduceFrame(BeginFrameAck(args, false));
+  support->DidNotProduceFrame(BeginFrameAck(last_sent_args, false));
 
   frametime += interval;
   args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,