Don't request pushing animation counts to compositor

Requesting pushing whether we have a requestAnimationFrame call or
animations to the compositor through AnimationHost requires a commit.
This leads to a lot of expensive messages even when nothing is changed
on the page. Since this is only used for metrics we should find a more
efficient way to track these changes.

Bug: 1083244
Change-Id: I093d036dafd0abf9e25b7e1592214a4c672866fd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216859Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773232}

cc/animation/animation_host.cc

@@ -219,6 +219,8 @@ void AnimationHost::SetNeedsCommit() {
 }
 
 void AnimationHost::SetNeedsPushProperties() {
+  if (needs_push_properties_)
+    return;
   needs_push_properties_ = true;
   if (mutator_host_client_)
     mutator_host_client_->SetMutatorsNeedCommit();
@@ -227,6 +229,14 @@ void AnimationHost::SetNeedsPushProperties() {
 void AnimationHost::PushPropertiesTo(MutatorHost* mutator_host_impl) {
   auto* host_impl = static_cast<AnimationHost*>(mutator_host_impl);
 
+  // Update animation counts and whether raf was requested. These explicitly
+  // do not request push properties and are pushed as part of the next commit
+  // when it happens as requesting a commit leads to performance issues:
+  // https://crbug.com/1083244
+  host_impl->main_thread_animations_count_ = main_thread_animations_count_;
+  host_impl->current_frame_had_raf_ = current_frame_had_raf_;
+  host_impl->next_frame_has_pending_raf_ = next_frame_has_pending_raf_;
+
   if (needs_push_properties_) {
     needs_push_properties_ = false;
     PushTimelinesToImplThread(host_impl);
@@ -289,9 +299,6 @@ void AnimationHost::PushPropertiesToImplThread(AnimationHost* host_impl) {
   // Update the impl-only scroll offset animations.
   scroll_offset_animations_->PushPropertiesTo(
       host_impl->scroll_offset_animations_impl_.get());
-  host_impl->main_thread_animations_count_ = main_thread_animations_count_;
-  host_impl->current_frame_had_raf_ = current_frame_had_raf_;
-  host_impl->next_frame_has_pending_raf_ = next_frame_has_pending_raf_;
 
   // The pending info list is cleared in LayerTreeHostImpl::CommitComplete
   // and should be empty when pushing properties.
@@ -763,26 +770,21 @@ void AnimationHost::SetAnimationCounts(
     size_t total_animations_count,
     bool current_frame_had_raf,
     bool next_frame_has_pending_raf) {
+  // Though these changes are pushed as part of AnimationHost::PushPropertiesTo
+  // we don't SetNeedsPushProperties as pushing the values requires a commit.
+  // Instead we allow them to be pushed whenever the next required commit
+  // happens to avoid unnecessary work. See https://crbug.com/1083244.
+
   // If an animation is being run on the compositor, it will have a ticking
   // Animation (which will have a corresponding impl-thread version). Therefore
   // to find the count of main-only animations, we can simply subtract the
   // number of ticking animations from the total count.
   size_t ticking_animations_count = ticking_animations_.size();
-  if (main_thread_animations_count_ !=
-      total_animations_count - ticking_animations_count) {
-    main_thread_animations_count_ =
-        total_animations_count - ticking_animations_count;
-    DCHECK_GE(main_thread_animations_count_, 0u);
-    SetNeedsPushProperties();
-  }
-  if (current_frame_had_raf != current_frame_had_raf_) {
-    current_frame_had_raf_ = current_frame_had_raf;
-    SetNeedsPushProperties();
-  }
-  if (next_frame_has_pending_raf != next_frame_has_pending_raf_) {
-    next_frame_has_pending_raf_ = next_frame_has_pending_raf;
-    SetNeedsPushProperties();
-  }
+  main_thread_animations_count_ =
+      total_animations_count - ticking_animations_count;
+  DCHECK_GE(main_thread_animations_count_, 0u);
+  current_frame_had_raf_ = current_frame_had_raf;
+  next_frame_has_pending_raf_ = next_frame_has_pending_raf;
 }
 
 size_t AnimationHost::MainThreadAnimationsCount() const {

cc/layers/layer_unittest.cc

@@ -1507,7 +1507,7 @@ TEST_F(LayerTest, SetLayerTreeHostNotUsingLayerListsManagesElementId) {
 
   // Expect additional calls due to has-animation check and initialization
   // of keyframes.
-  EXPECT_CALL(*layer_tree_host_, SetNeedsCommit()).Times(7);
+  EXPECT_CALL(*layer_tree_host_, SetNeedsCommit()).Times(3);
   scoped_refptr<AnimationTimeline> timeline =
       AnimationTimeline::Create(AnimationIdProvider::NextTimelineId());
   animation_host_->AddAnimationTimeline(timeline);
@@ -1526,6 +1526,19 @@ TEST_F(LayerTest, SetLayerTreeHostNotUsingLayerListsManagesElementId) {
   EXPECT_EQ(nullptr, layer_tree_host_->LayerByElementId(element_id));
 }
 
+// Triggering a commit to push animation counts and raf presence to the
+// compositor is expensive and updated counts can wait until the next
+// commit to be pushed. See https://crbug.com/1083244.
+TEST_F(LayerTest, PushAnimationCountsLazily) {
+  EXPECT_CALL(*layer_tree_host_, SetNeedsCommit()).Times(0);
+  animation_host_->SetAnimationCounts(0, /* current_frame_had_raf = */ true,
+                                      /* next_frame_has_pending_raf = */ true);
+  EXPECT_FALSE(host_impl_.animation_host()->CurrentFrameHadRAF());
+  EXPECT_FALSE(animation_host_->needs_push_properties());
+  animation_host_->PushPropertiesTo(host_impl_.animation_host());
+  EXPECT_TRUE(host_impl_.animation_host()->CurrentFrameHadRAF());
+}
+
 TEST_F(LayerTest, SetElementIdNotUsingLayerLists) {
   scoped_refptr<Layer> test_layer = Layer::Create();
   test_layer->SetLayerTreeHost(layer_tree_host_.get());