Bug 1455302 - Allow scheduling updater thread tasks before we have the updater thread id. r=botond
authorKartikaya Gupta <kgupta@mozilla.com>
Thu, 19 Apr 2018 10:09:59 -0400
changeset 468207 8450bccb1873ca29bd5f226af0dcf5d6186612f0
parent 468206 fbfbfe496fcb39556a00f9209116b8374e79bcb7
child 468208 9bb64b4e34ce2ec456f4e6084a7b358e5ca500e0
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1455302
milestone61.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 1455302 - Allow scheduling updater thread tasks before we have the updater thread id. r=botond This is possible if we just let the APZUpdater know during construction if WR is enabled or not, and that information combined with the pref will allow it to know whether to use the scene builder thread task queue or just use the compositor thread as the updater thread. MozReview-Commit-ID: 7IGMMtl7iFP
gfx/layers/apz/public/APZUpdater.h
gfx/layers/apz/src/APZUpdater.cpp
gfx/layers/apz/test/gtest/APZCBasicTester.h
gfx/layers/apz/test/gtest/APZCTreeManagerTester.h
gfx/layers/ipc/CompositorBridgeParent.cpp
gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp
--- a/gfx/layers/apz/public/APZUpdater.h
+++ b/gfx/layers/apz/public/APZUpdater.h
@@ -39,17 +39,18 @@ class WebRenderScrollData;
  * response to IPC messages. The method implementations internally redispatch
  * themselves to the updater thread in the case where the compositor thread
  * is not the updater thread.
  */
 class APZUpdater {
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(APZUpdater)
 
 public:
-  explicit APZUpdater(const RefPtr<APZCTreeManager>& aApz);
+  APZUpdater(const RefPtr<APZCTreeManager>& aApz,
+             bool aIsUsingWebRender);
 
   bool HasTreeManager(const RefPtr<APZCTreeManager>& aApz);
   void SetWebRenderWindowId(const wr::WindowId& aWindowId);
 
   /**
    * This function is invoked from rust on the scene builder thread when it
    * is created. It effectively tells the APZUpdater "the current thread is
    * the updater thread for this window id" and allows APZUpdater to remember
@@ -140,16 +141,17 @@ protected:
 
   bool UsingWebRenderUpdaterThread() const;
   static already_AddRefed<APZUpdater> GetUpdater(const wr::WrWindowId& aWindowId);
 
   void ProcessQueue();
 
 private:
   RefPtr<APZCTreeManager> mApz;
+  bool mIsUsingWebRender;
 
   // Map from layers id to WebRenderScrollData. This can only be touched on
   // the updater thread.
   std::unordered_map<LayersId,
                      WebRenderScrollData,
                      LayersId::HashFn> mScrollData;
 
   // Stores epoch state for a particular layers id. This structure is only
@@ -188,30 +190,24 @@ private:
   // be used while holding the sWindowIdLock. Note that we use a StaticAutoPtr
   // wrapper on sWindowIdMap to avoid a static initializer for the unordered_map.
   // This also avoids the initializer/memory allocation in cases where we're
   // not using WebRender.
   static StaticMutex sWindowIdLock;
   static StaticAutoPtr<std::unordered_map<uint64_t, APZUpdater*>> sWindowIdMap;
   Maybe<wr::WrWindowId> mWindowId;
 
+  // Lock used to protected mUpdaterThreadId;
+  mutable Mutex mThreadIdLock;
   // If WebRender and async scene building are enabled, this holds the thread id
   // of the scene builder thread (which is the updater thread) for the
   // compositor associated with this APZUpdater instance. It may be populated
   // even if async scene building is not enabled, but in that case we don't
   // care about the contents.
-  // This is written to once during init and never cleared, and so reading it
-  // from multiple threads during normal operation (after initialization)
-  // without locking should be fine.
   Maybe<PlatformThreadId> mUpdaterThreadId;
-#ifdef DEBUG
-  // This flag is used to ensure that we don't ever try to do updater-thread
-  // stuff before the updater thread has been properly initialized.
-  mutable bool mUpdaterThreadQueried;
-#endif
 
   // Helper struct that pairs each queued runnable with the layers id that it
   // is associated with. This allows us to easily implement the conceptual
   // separation of mUpdaterQueue into independent queues per layers id.
   struct QueuedTask {
     LayersId mLayersId;
     RefPtr<Runnable> mRunnable;
   };
--- a/gfx/layers/apz/src/APZUpdater.cpp
+++ b/gfx/layers/apz/src/APZUpdater.cpp
@@ -17,21 +17,21 @@
 
 namespace mozilla {
 namespace layers {
 
 StaticMutex APZUpdater::sWindowIdLock;
 StaticAutoPtr<std::unordered_map<uint64_t, APZUpdater*>> APZUpdater::sWindowIdMap;
 
 
-APZUpdater::APZUpdater(const RefPtr<APZCTreeManager>& aApz)
+APZUpdater::APZUpdater(const RefPtr<APZCTreeManager>& aApz,
+                       bool aIsUsingWebRender)
   : mApz(aApz)
-#ifdef DEBUG
-  , mUpdaterThreadQueried(false)
-#endif
+  , mIsUsingWebRender(aIsUsingWebRender)
+  , mThreadIdLock("APZUpdater::ThreadIdLock")
   , mQueueLock("APZUpdater::QueueLock")
 {
   MOZ_ASSERT(aApz);
   mApz->SetUpdater(this);
 }
 
 APZUpdater::~APZUpdater()
 {
@@ -62,18 +62,17 @@ APZUpdater::SetWebRenderWindowId(const w
   }
   (*sWindowIdMap)[wr::AsUint64(aWindowId)] = this;
 }
 
 /*static*/ void
 APZUpdater::SetUpdaterThread(const wr::WrWindowId& aWindowId)
 {
   if (RefPtr<APZUpdater> updater = GetUpdater(aWindowId)) {
-    // Ensure nobody tried to use the updater thread before this point.
-    MOZ_ASSERT(!updater->mUpdaterThreadQueried);
+    MutexAutoLock lock(updater->mThreadIdLock);
     updater->mUpdaterThreadId = Some(PlatformThread::CurrentId());
   }
 }
 
 /*static*/ void
 APZUpdater::PrepareForSceneSwap(const wr::WrWindowId& aWindowId)
 {
   if (RefPtr<APZUpdater> updater = GetUpdater(aWindowId)) {
@@ -334,16 +333,23 @@ APZUpdater::AssertOnUpdaterThread() cons
   }
 }
 
 void
 APZUpdater::RunOnUpdaterThread(LayersId aLayersId, already_AddRefed<Runnable> aTask)
 {
   RefPtr<Runnable> task = aTask;
 
+  // In the scenario where UsingWebRenderUpdaterThread() is true, this function
+  // might get called early (before mUpdaterThreadId is set). In that case
+  // IsUpdaterThread() will return false and we'll queue the task onto
+  // mUpdaterQueue. This is fine; the task is still guaranteed to run (barring
+  // catastrophic failure) because the WakeSceneBuilder call will still trigger
+  // the callback to run tasks.
+
   if (IsUpdaterThread()) {
     task->Run();
     return;
   }
 
   if (UsingWebRenderUpdaterThread()) {
     // If the updater thread is a WebRender thread, and we're not on it
     // right now, save the task in the queue. We will run tasks from the queue
@@ -375,17 +381,22 @@ APZUpdater::RunOnUpdaterThread(LayersId 
     NS_WARNING("Dropping task posted to updater thread");
   }
 }
 
 bool
 APZUpdater::IsUpdaterThread() const
 {
   if (UsingWebRenderUpdaterThread()) {
-    return PlatformThread::CurrentId() == *mUpdaterThreadId;
+    // If the updater thread id isn't set yet then we cannot be running on the
+    // updater thread (because we will have the thread id before we run any
+    // C++ code on it, and this function is only ever invoked from C++ code),
+    // so return false in that scenario.
+    MutexAutoLock lock(mThreadIdLock);
+    return mUpdaterThreadId && PlatformThread::CurrentId() == *mUpdaterThreadId;
   }
   return CompositorThreadHolder::IsInCompositorThread();
 }
 
 void
 APZUpdater::RunOnControllerThread(LayersId aLayersId, already_AddRefed<Runnable> aTask)
 {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
@@ -394,31 +405,17 @@ APZUpdater::RunOnControllerThread(Layers
       "APZUpdater::RunOnControllerThread",
       &APZThreadUtils::RunOnControllerThread,
       Move(aTask)));
 }
 
 bool
 APZUpdater::UsingWebRenderUpdaterThread() const
 {
-  if (!gfxPrefs::WebRenderAsyncSceneBuild()) {
-    return false;
-  }
-  // If mUpdaterThreadId is not set at the point that this is called, then
-  // that means that either (a) WebRender is not enabled for the compositor
-  // to which this APZUpdater is attached or (b) we are attempting to do
-  // something updater-related before WebRender is up and running. In case
-  // (a) falling back to the compositor thread is correct, and in case (b)
-  // we should stop doing the updater-related thing so early. We catch this
-  // case by setting the mUpdaterThreadQueried flag and asserting on WR
-  // initialization.
-#ifdef DEBUG
-  mUpdaterThreadQueried = true;
-#endif
-  return mUpdaterThreadId.isSome();
+  return (mIsUsingWebRender && gfxPrefs::WebRenderAsyncSceneBuild());
 }
 
 /*static*/ already_AddRefed<APZUpdater>
 APZUpdater::GetUpdater(const wr::WrWindowId& aWindowId)
 {
   RefPtr<APZUpdater> updater;
   StaticMutexAutoLock lock(sWindowIdLock);
   if (sWindowIdMap) {
--- a/gfx/layers/apz/test/gtest/APZCBasicTester.h
+++ b/gfx/layers/apz/test/gtest/APZCBasicTester.h
@@ -26,17 +26,17 @@ public:
 protected:
   virtual void SetUp()
   {
     gfxPrefs::GetSingleton();
     APZThreadUtils::SetThreadAssertionsEnabled(false);
     APZThreadUtils::SetControllerThread(MessageLoop::current());
 
     tm = new TestAPZCTreeManager(mcc);
-    updater = new APZUpdater(tm);
+    updater = new APZUpdater(tm, false);
     sampler = new APZSampler(tm);
     apzc = new TestAsyncPanZoomController(LayersId{0}, mcc, tm, mGestureBehavior);
     apzc->SetFrameMetrics(TestFrameMetrics());
     apzc->GetScrollMetadata().SetIsLayersIdRoot(true);
   }
 
   /**
    * Get the APZC's scroll range in CSS pixels.
--- a/gfx/layers/apz/test/gtest/APZCTreeManagerTester.h
+++ b/gfx/layers/apz/test/gtest/APZCTreeManagerTester.h
@@ -22,17 +22,17 @@ class APZCTreeManagerTester : public APZ
 protected:
   virtual void SetUp() {
     gfxPrefs::GetSingleton();
     gfxPlatform::GetPlatform();
     APZThreadUtils::SetThreadAssertionsEnabled(false);
     APZThreadUtils::SetControllerThread(MessageLoop::current());
 
     manager = new TestAPZCTreeManager(mcc);
-    updater = new APZUpdater(manager);
+    updater = new APZUpdater(manager, false);
     sampler = new APZSampler(manager);
   }
 
   virtual void TearDown() {
     while (mcc->RunThroughDelayedTasks());
     manager->ClearTree();
     manager->ClearContentController();
   }
--- a/gfx/layers/ipc/CompositorBridgeParent.cpp
+++ b/gfx/layers/ipc/CompositorBridgeParent.cpp
@@ -381,17 +381,17 @@ CompositorBridgeParent::Initialize()
              "The compositor thread must be Initialized before instanciating a CompositorBridgeParent.");
 
   if (mOptions.UseAPZ()) {
     MOZ_ASSERT(!mApzcTreeManager);
     MOZ_ASSERT(!mApzSampler);
     MOZ_ASSERT(!mApzUpdater);
     mApzcTreeManager = new APZCTreeManager(mRootLayerTreeID);
     mApzSampler = new APZSampler(mApzcTreeManager);
-    mApzUpdater = new APZUpdater(mApzcTreeManager);
+    mApzUpdater = new APZUpdater(mApzcTreeManager, mOptions.UseWebRender());
   }
 
   mCompositorBridgeID = 0;
   // FIXME: This holds on the the fact that right now the only thing that
   // can destroy this instance is initialized on the compositor thread after
   // this task has been processed.
   MOZ_ASSERT(CompositorLoop());
   CompositorLoop()->PostTask(NewRunnableFunction("AddCompositorRunnable",
--- a/gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp
+++ b/gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp
@@ -130,17 +130,17 @@ CrossProcessCompositorBridgeParent::Allo
   // If the widget has shutdown its compositor, we may not have had a chance yet
   // to unmap our layers id, and we could get here without a parent compositor.
   // In this case return an empty APZCTM.
   if (!state.mParent) {
     // Note: we immediately call ClearTree since otherwise the APZCTM will
     // retain a reference to itself, through the checkerboard observer.
     LayersId dummyId{0};
     RefPtr<APZCTreeManager> temp = new APZCTreeManager(dummyId);
-    RefPtr<APZUpdater> tempUpdater = new APZUpdater(temp);
+    RefPtr<APZUpdater> tempUpdater = new APZUpdater(temp, false);
     tempUpdater->ClearTree(dummyId);
     return new APZCTreeManagerParent(aLayersId, temp, tempUpdater);
   }
 
   state.mParent->AllocateAPZCTreeManagerParent(lock, aLayersId, state);
   return state.mApzcTreeManagerParent;
 }
 bool