Bug 1490117 - Rollback WrEpoch if TransactionBuilder does not have ResourceUpdates in RecvEmptyTransaction() r=mattwoodrow
authorsotaro <sotaro.ikeda.g@gmail.com>
Mon, 26 Nov 2018 16:08:49 +0900
changeset 447946 3214fd9390a0cf9e74599014d8c325ea1b7850fc
parent 447937 def0fd8429f955dcd8f29ae54cd2e2e5cd28032d
child 447947 14ae1910a4f5dd1cdea7fcb4855a3aa86ed3731b
child 447971 45b378c260d9308b6b067e42da3412961d431128
push id35099
push userncsoregi@mozilla.com
push dateMon, 26 Nov 2018 09:47:34 +0000
treeherdermozilla-central@14ae1910a4f5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow
bugs1490117
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1490117 - Rollback WrEpoch if TransactionBuilder does not have ResourceUpdates in RecvEmptyTransaction() r=mattwoodrow
gfx/layers/wr/WebRenderBridgeParent.cpp
gfx/layers/wr/WebRenderBridgeParent.h
gfx/webrender_bindings/WebRenderAPI.cpp
gfx/webrender_bindings/WebRenderAPI.h
gfx/webrender_bindings/src/bindings.rs
gfx/webrender_bindings/webrender_ffi_generated.h
--- a/gfx/layers/wr/WebRenderBridgeParent.cpp
+++ b/gfx/layers/wr/WebRenderBridgeParent.cpp
@@ -1080,48 +1080,57 @@ WebRenderBridgeParent::RecvEmptyTransact
 
   UpdateAPZFocusState(aFocusTarget);
   if (!aUpdates.empty()) {
     // aUpdates is moved into this function but that is not reflected by the
     // function signature due to the way the IPDL generator works. We remove the
     // const so that we can move this structure all the way to the desired
     // destination.
     UpdateAPZScrollOffsets(std::move(const_cast<ScrollUpdatesMap&>(aUpdates)), aPaintSequenceNumber);
-    scheduleComposite = true;
   }
 
   wr::TransactionBuilder txn;
   txn.SetLowPriority(!IsRootWebRenderBridgeParent());
-  if (!aResourceUpdates.IsEmpty()) {
-    scheduleComposite = true;
-  }
+
+  // Update WrEpoch for UpdateResources() and ProcessWebRenderParentCommands().
+  // WrEpoch is used to manage ExternalImages lifetimes in AsyncImagePipelineManager.
+  Unused << GetNextWrEpoch();
 
   if (!UpdateResources(aResourceUpdates, aSmallShmems, aLargeShmems, txn)) {
     return IPC_FAIL(this, "Failed to deserialize resource updates");
   }
 
   if (!aCommands.IsEmpty()) {
     mAsyncImageManager->SetCompositionTime(TimeStamp::Now());
-    wr::Epoch wrEpoch = GetNextWrEpoch();
-    txn.UpdateEpoch(mPipelineId, wrEpoch);
     if (!ProcessWebRenderParentCommands(aCommands, txn)) {
       return IPC_FAIL(this, "Invalid parent command found");
     }
-    if (ShouldParentObserveEpoch()) {
-      txn.Notify(
-        wr::Checkpoint::SceneBuilt,
-        MakeUnique<ScheduleObserveLayersUpdate>(
-          mCompositorBridge,
-          GetLayersId(),
-          mChildLayersObserverEpoch,
-          true
-        )
-      );
-    }
+  }
 
+  if (ShouldParentObserveEpoch()) {
+    txn.Notify(
+      wr::Checkpoint::SceneBuilt,
+      MakeUnique<ScheduleObserveLayersUpdate>(
+        mCompositorBridge,
+        GetLayersId(),
+        mChildLayersObserverEpoch,
+        true
+      )
+    );
+  }
+
+  if (txn.IsResourceUpdatesEmpty()) {
+    // If TransactionBuilder does not have resource updates nor display list,
+    // ScheduleGenerateFrame is not triggered via SceneBuilder and there is no
+    // need to update WrEpoch.
+    // Then we want to rollback WrEpoch. See Bug 1490117.
+    RollbackWrEpoch();
+  } else {
+    // There are resource updates, then we update Epoch of transaction.
+    txn.UpdateEpoch(mPipelineId, mWrEpoch);
     scheduleComposite = true;
   }
 
   if (!txn.IsEmpty()) {
     mApi->SendTransaction(txn);
   }
 
   bool sendDidComposite = true;
@@ -1144,16 +1153,20 @@ WebRenderBridgeParent::RecvEmptyTransact
                            aRefreshStartTime,
                            aTxnStartTime,
                            aTxnURL,
                            aFwdTime,
                            /* aIsFirstPaint */false,
                            /* aUseForTelemetry */scheduleComposite);
 
   if (scheduleComposite) {
+    // This is actually not necessary, since ScheduleGenerateFrame() is triggered
+    // via SceneBuilder thread. But if we remove it, it causes talos regression.
+    // The SceneBuilder thread seems not trigger next vsync right away.
+    // For now, we call ScheduleGenerateFrame() here.
     ScheduleGenerateFrame();
   } else if (sendDidComposite) {
     // The only thing in the pending transaction id queue should be the entry
     // we just added, and now we're going to pretend we rendered it
     MOZ_ASSERT(mPendingTransactionIds.size() == 1);
     if (CompositorBridgeParent* cbp = GetRootCompositorBridgeParent()) {
       TimeStamp now = TimeStamp::Now();
       cbp->NotifyPipelineRendered(mPipelineId, mWrEpoch, now, now, now);
@@ -2227,16 +2240,23 @@ wr::Epoch
 WebRenderBridgeParent::GetNextWrEpoch()
 {
   MOZ_RELEASE_ASSERT(mWrEpoch.mHandle != UINT32_MAX);
   mWrEpoch.mHandle++;
   return mWrEpoch;
 }
 
 void
+WebRenderBridgeParent::RollbackWrEpoch()
+{
+  MOZ_RELEASE_ASSERT(mWrEpoch.mHandle != 0);
+  mWrEpoch.mHandle--;
+}
+
+void
 WebRenderBridgeParent::ExtractImageCompositeNotifications(nsTArray<ImageCompositeNotificationInfo>* aNotifications)
 {
   MOZ_ASSERT(IsRootWebRenderBridgeParent());
   if (mDestroyed) {
     return;
   }
   mAsyncImageManager->FlushImageNotifications(aNotifications);
 }
--- a/gfx/layers/wr/WebRenderBridgeParent.h
+++ b/gfx/layers/wr/WebRenderBridgeParent.h
@@ -290,16 +290,21 @@ private:
   CompositorBridgeParent* GetRootCompositorBridgeParent() const;
 
   RefPtr<WebRenderBridgeParent> GetRootWebRenderBridgeParent() const;
 
   // Tell APZ what the subsequent sampling's timestamp should be.
   void SetAPZSampleTime();
 
   wr::Epoch GetNextWrEpoch();
+  // This function is expected to be used when GetNextWrEpoch() is called,
+  // but TransactionBuilder does not have resource updates nor display list.
+  // In this case, ScheduleGenerateFrame is not triggered via SceneBuilder.
+  // Then we want to rollback WrEpoch. See Bug 1490117.
+  void RollbackWrEpoch();
 
   void FlushSceneBuilds();
   void FlushFrameGeneration();
   void FlushFramePresentation();
 
   void MaybeGenerateFrame(bool aForceGenerateFrame);
 
 private:
--- a/gfx/webrender_bindings/WebRenderAPI.cpp
+++ b/gfx/webrender_bindings/WebRenderAPI.cpp
@@ -231,16 +231,22 @@ TransactionBuilder::UpdateDynamicPropert
 }
 
 bool
 TransactionBuilder::IsEmpty() const
 {
   return wr_transaction_is_empty(mTxn);
 }
 
+bool
+TransactionBuilder::IsResourceUpdatesEmpty() const
+{
+  return wr_transaction_resource_updates_is_empty(mTxn);
+}
+
 void
 TransactionBuilder::SetWindowParameters(const LayoutDeviceIntSize& aWindowSize,
                                         const LayoutDeviceIntRect& aDocumentRect)
 {
   wr::DeviceIntSize wrWindowSize;
   wrWindowSize.width = aWindowSize.width;
   wrWindowSize.height = aWindowSize.height;
   wr::DeviceIntRect wrDocRect;
--- a/gfx/webrender_bindings/WebRenderAPI.h
+++ b/gfx/webrender_bindings/WebRenderAPI.h
@@ -98,16 +98,18 @@ public:
                            const LayoutDeviceIntRect& aDocRect);
 
   void UpdateScrollPosition(const wr::WrPipelineId& aPipelineId,
                             const layers::ScrollableLayerGuid::ViewID& aScrollId,
                             const wr::LayoutPoint& aScrollPosition);
 
   bool IsEmpty() const;
 
+  bool IsResourceUpdatesEmpty() const;
+
   void AddImage(wr::ImageKey aKey,
                 const ImageDescriptor& aDescriptor,
                 wr::Vec<uint8_t>& aBytes);
 
   void AddBlobImage(wr::BlobImageKey aKey,
                     const ImageDescriptor& aDescriptor,
                     wr::Vec<uint8_t>& aBytes);
 
--- a/gfx/webrender_bindings/src/bindings.rs
+++ b/gfx/webrender_bindings/src/bindings.rs
@@ -1215,16 +1215,21 @@ pub extern "C" fn wr_transaction_set_low
 }
 
 #[no_mangle]
 pub extern "C" fn wr_transaction_is_empty(txn: &Transaction) -> bool {
     txn.is_empty()
 }
 
 #[no_mangle]
+pub extern "C" fn wr_transaction_resource_updates_is_empty(txn: &Transaction) -> bool {
+    txn.resource_updates.is_empty()
+}
+
+#[no_mangle]
 pub extern "C" fn wr_transaction_notify(txn: &mut Transaction, when: Checkpoint, event: usize) {
     struct GeckoNotification(usize);
     impl NotificationHandler for GeckoNotification {
         fn notify(&self, when: Checkpoint) {
             unsafe {
                 wr_transaction_notification_notified(self.0, when);
             }
         }
--- a/gfx/webrender_bindings/webrender_ffi_generated.h
+++ b/gfx/webrender_bindings/webrender_ffi_generated.h
@@ -629,32 +629,32 @@ struct TypedPoint2D {
   bool operator==(const TypedPoint2D& aOther) const {
     return x == aOther.x &&
            y == aOther.y;
   }
 };
 
 using WorldPoint = TypedPoint2D<float, WorldPixel>;
 
+struct WrDebugFlags {
+  uint32_t mBits;
+
+  bool operator==(const WrDebugFlags& aOther) const {
+    return mBits == aOther.mBits;
+  }
+};
+
 struct WrClipId {
   uintptr_t id;
 
   bool operator==(const WrClipId& aOther) const {
     return id == aOther.id;
   }
 };
 
-struct WrDebugFlags {
-  uint32_t mBits;
-
-  bool operator==(const WrDebugFlags& aOther) const {
-    return mBits == aOther.mBits;
-  }
-};
-
 // A 2d Rectangle optionally tagged with a unit.
 template<typename T, typename U>
 struct TypedRect {
   TypedPoint2D<T, U> origin;
   TypedSize2D<T, U> size;
 
   bool operator==(const TypedRect& aOther) const {
     return origin == aOther.origin &&
@@ -1908,16 +1908,20 @@ void wr_transaction_pinch_zoom(Transacti
 WR_FUNC;
 
 WR_INLINE
 void wr_transaction_remove_pipeline(Transaction *aTxn,
                                     WrPipelineId aPipelineId)
 WR_FUNC;
 
 WR_INLINE
+bool wr_transaction_resource_updates_is_empty(const Transaction *aTxn)
+WR_FUNC;
+
+WR_INLINE
 void wr_transaction_scroll_layer(Transaction *aTxn,
                                  WrPipelineId aPipelineId,
                                  uint64_t aScrollId,
                                  LayoutPoint aNewScrollOrigin)
 WR_FUNC;
 
 WR_INLINE
 void wr_transaction_set_display_list(Transaction *aTxn,