Bug 1425257: Introduce a global lock to protect the dependency graph between DrawTargets. r=lsalzman
authorBas Schouten <bschouten@mozilla.com>
Tue, 06 Feb 2018 03:08:04 +0100
changeset 402621 e30390f2f53a505a1dc514dad36c181d75e4c83c
parent 402620 0104a3c366e71354eb34fecf1e280c75f2aa276c
child 402622 0790ec12200d5f663dd000a6ef3c72c795ca4ead
child 402660 637dffdef3fb7ae1794f14fabcb95647e0566cc5
push id33393
push userrgurzau@mozilla.com
push dateTue, 06 Feb 2018 21:54:26 +0000
treeherdermozilla-central@0790ec12200d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslsalzman
bugs1425257
milestone60.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 1425257: Introduce a global lock to protect the dependency graph between DrawTargets. r=lsalzman This patch takes the safest route for securing modifications to the dependency graph for D2D DrawTargets. It's possible a slightly optimal approach is possible, however lock contention should be rare and I believe this is the safest and most upliftable approach. MozReview-Commit-ID: HGfSdEp2U5N
gfx/2d/2D.h
gfx/2d/DrawTargetD2D1.cpp
gfx/2d/DrawTargetD2D1.h
gfx/2d/Factory.cpp
--- a/gfx/2d/2D.h
+++ b/gfx/2d/2D.h
@@ -1750,16 +1750,19 @@ private:
   static StaticRefPtr<ID3D11Device> mD3D11Device;
   static StaticRefPtr<IDWriteFactory> mDWriteFactory;
   static bool mDWriteFactoryInitialized;
 
 protected:
   // This guards access to the singleton devices above, as well as the
   // singleton devices in DrawTargetD2D1.
   static StaticMutex mDeviceLock;
+  // This synchronizes access between different D2D drawtargets and their
+  // implied dependency graph.
+  static StaticMutex mDTDependencyLock;
 
   friend class DrawTargetD2D1;
 #endif
 
 private:
   static DrawEventRecorder *mRecorder;
 };
 
--- a/gfx/2d/DrawTargetD2D1.cpp
+++ b/gfx/2d/DrawTargetD2D1.cpp
@@ -71,26 +71,31 @@ DrawTargetD2D1::~DrawTargetD2D1()
 
   if (mDC && IsDeviceContextValid()) {
     // The only way mDC can be null is if Init failed, but it can happen and the
     // destructor is the only place where we need to check for it since the
     // DrawTarget will destroyed right after Init fails.
     mDC->EndDraw();
   }
 
-  // Targets depending on us can break that dependency, since we're obviously not going to
-  // be modified in the future.
-  for (auto iter = mDependentTargets.begin();
-       iter != mDependentTargets.end(); iter++) {
-    (*iter)->mDependingOnTargets.erase(this);
-  }
-  // Our dependencies on other targets no longer matter.
-  for (TargetSet::iterator iter = mDependingOnTargets.begin();
-       iter != mDependingOnTargets.end(); iter++) {
-    (*iter)->mDependentTargets.erase(this);
+  {
+    // Until this point in the destructor it -must- still be valid for FlushInternal
+    // to be called on this.
+    StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+    // Targets depending on us can break that dependency, since we're obviously not going to
+    // be modified in the future.
+    for (auto iter = mDependentTargets.begin();
+      iter != mDependentTargets.end(); iter++) {
+      (*iter)->mDependingOnTargets.erase(this);
+    }
+    // Our dependencies on other targets no longer matter.
+    for (TargetSet::iterator iter = mDependingOnTargets.begin();
+      iter != mDependingOnTargets.end(); iter++) {
+      (*iter)->mDependentTargets.erase(this);
+    }
   }
 }
 
 already_AddRefed<SourceSurface>
 DrawTargetD2D1::Snapshot()
 {
   MutexAutoLock lock(*mSnapshotLock);
   if (mSnapshot) {
@@ -151,38 +156,17 @@ DrawTargetD2D1::IntoLuminanceSource(Lumi
 // this can cause issues with memory usage (see bug 1238328). EndDraw/BeginDraw
 // are expensive though, especially relatively when little work is done, so
 // we try to reduce the amount of times we execute these purges.
 static const uint32_t kPushedLayersBeforePurge = 25;
 
 void
 DrawTargetD2D1::Flush()
 {
-  if (IsDeviceContextValid()) {
-    if ((mUsedCommandListsSincePurge >= kPushedLayersBeforePurge) &&
-        mPushedLayers.size() == 1) {
-      // It's important to pop all clips as otherwise layers can forget about
-      // their clip when doing an EndDraw. When we have layers pushed we cannot
-      // easily pop all underlying clips to delay the purge until we have no
-      // layers pushed.
-      PopAllClips();
-      mUsedCommandListsSincePurge = 0;
-      mDC->EndDraw();
-      mDC->BeginDraw();
-    } else {
-      mDC->Flush();
-    }
-  }
-
-  // We no longer depend on any target.
-  for (TargetSet::iterator iter = mDependingOnTargets.begin();
-       iter != mDependingOnTargets.end(); iter++) {
-    (*iter)->mDependentTargets.erase(this);
-  }
-  mDependingOnTargets.clear();
+  FlushInternal();
 }
 
 void
 DrawTargetD2D1::DrawSurface(SourceSurface *aSurface,
                             const Rect &aDest,
                             const Rect &aSource,
                             const DrawSurfaceOptions &aSurfOptions,
                             const DrawOptions &aOptions)
@@ -1267,38 +1251,77 @@ DrawTargetD2D1::CleanupD2D()
   if (mFactory) {
     RadialGradientEffectD2D1::Unregister(mFactory);
     ExtendInputEffectD2D1::Unregister(mFactory);
     mFactory = nullptr;
   }
 }
 
 void
+DrawTargetD2D1::FlushInternal(bool aHasDependencyMutex /* = false */)
+{
+  if (IsDeviceContextValid()) {
+    if ((mUsedCommandListsSincePurge >= kPushedLayersBeforePurge) &&
+      mPushedLayers.size() == 1) {
+      // It's important to pop all clips as otherwise layers can forget about
+      // their clip when doing an EndDraw. When we have layers pushed we cannot
+      // easily pop all underlying clips to delay the purge until we have no
+      // layers pushed.
+      PopAllClips();
+      mUsedCommandListsSincePurge = 0;
+      mDC->EndDraw();
+      mDC->BeginDraw();
+    }
+    else {
+      mDC->Flush();
+    }
+  }
+
+  Maybe<StaticMutexAutoLock> lock;
+
+  if (!aHasDependencyMutex) {
+    lock.emplace(Factory::mDTDependencyLock);
+  }
+
+  Factory::mDTDependencyLock.AssertCurrentThreadOwns();
+  // We no longer depend on any target.
+  for (TargetSet::iterator iter = mDependingOnTargets.begin();
+    iter != mDependingOnTargets.end(); iter++) {
+    (*iter)->mDependentTargets.erase(this);
+  }
+  mDependingOnTargets.clear();
+}
+
+void
 DrawTargetD2D1::MarkChanged()
 {
   if (mSnapshot) {
     MutexAutoLock lock(*mSnapshotLock);
     if (mSnapshot->hasOneRef()) {
       // Just destroy it, since no-one else knows about it.
       mSnapshot = nullptr;
     } else {
       mSnapshot->DrawTargetWillChange();
       // The snapshot will no longer depend on this target.
       MOZ_ASSERT(!mSnapshot);
     }
   }
-  if (mDependentTargets.size()) {
-    // Copy mDependentTargets since the Flush()es below will modify it.
-    TargetSet tmpTargets = mDependentTargets;
-    for (TargetSet::iterator iter = tmpTargets.begin();
-         iter != tmpTargets.end(); iter++) {
-      (*iter)->Flush();
+
+  {
+    StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+    if (mDependentTargets.size()) {
+      // Copy mDependentTargets since the Flush()es below will modify it.
+      TargetSet tmpTargets = mDependentTargets;
+      for (TargetSet::iterator iter = tmpTargets.begin();
+        iter != tmpTargets.end(); iter++) {
+        (*iter)->FlushInternal(true);
+      }
+      // The Flush() should have broken all dependencies on this target.
+      MOZ_ASSERT(!mDependentTargets.size());
     }
-    // The Flush() should have broken all dependencies on this target.
-    MOZ_ASSERT(!mDependentTargets.size());
   }
 }
 
 bool
 DrawTargetD2D1::ShouldClipTemporarySurfaceDrawing(CompositionOp aOp,
                                                   const Pattern& aPattern,
                                                   bool aClipIsComplex)
 {
@@ -1459,19 +1482,28 @@ DrawTargetD2D1::FinalizeDrawing(Composit
   radialGradientEffect->SetInput(0, source);
 
   mDC->DrawImage(radialGradientEffect, D2D1_INTERPOLATION_MODE_NEAREST_NEIGHBOR, D2DCompositionMode(aOp));
 }
 
 void
 DrawTargetD2D1::AddDependencyOnSource(SourceSurfaceD2D1* aSource)
 {
-  if (aSource->mDrawTarget && !mDependingOnTargets.count(aSource->mDrawTarget)) {
-    aSource->mDrawTarget->mDependentTargets.insert(this);
-    mDependingOnTargets.insert(aSource->mDrawTarget);
+  Maybe<MutexAutoLock> snapshotLock;
+  // We grab the SnapshotLock as well, this guaranteeds aSource->mDrawTarget
+  // cannot be cleared in between the if statement and the dereference.
+  if (aSource->mSnapshotLock) {
+    snapshotLock.emplace(*aSource->mSnapshotLock);
+  }
+  {
+    StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+    if (aSource->mDrawTarget && !mDependingOnTargets.count(aSource->mDrawTarget)) {
+      aSource->mDrawTarget->mDependentTargets.insert(this);
+      mDependingOnTargets.insert(aSource->mDrawTarget);
+    }
   }
 }
 
 static D2D1_RECT_F
 IntersectRect(const D2D1_RECT_F& aRect1, const D2D1_RECT_F& aRect2)
 {
   D2D1_RECT_F result;
   result.left = max(aRect1.left, aRect2.left);
--- a/gfx/2d/DrawTargetD2D1.h
+++ b/gfx/2d/DrawTargetD2D1.h
@@ -168,16 +168,18 @@ public:
   }
 
   static uint64_t mVRAMUsageDT;
   static uint64_t mVRAMUsageSS;
 
 private:
   friend class SourceSurfaceD2D1;
 
+  void FlushInternal(bool aHasDependencyMutex = false);
+
   typedef std::unordered_set<DrawTargetD2D1*> TargetSet;
 
   // This function will mark the surface as changing, and make sure any
   // copy-on-write snapshots are notified.
   void MarkChanged();
   bool ShouldClipTemporarySurfaceDrawing(CompositionOp aOp, const Pattern& aPattern, bool aClipIsComplex);
   void PrepareForDrawing(CompositionOp aOp, const Pattern &aPattern);
   void FinalizeDrawing(CompositionOp aOp, const Pattern &aPattern);
--- a/gfx/2d/Factory.cpp
+++ b/gfx/2d/Factory.cpp
@@ -217,16 +217,17 @@ Mutex* Factory::mFTLock = nullptr;
 #ifdef WIN32
 // Note: mDeviceLock must be held when mutating these values.
 static uint32_t mDeviceSeq = 0;
 StaticRefPtr<ID3D11Device> Factory::mD3D11Device;
 StaticRefPtr<ID2D1Device> Factory::mD2D1Device;
 StaticRefPtr<IDWriteFactory> Factory::mDWriteFactory;
 bool Factory::mDWriteFactoryInitialized = false;
 StaticMutex Factory::mDeviceLock;
+StaticMutex Factory::mDTDependencyLock;
 #endif
 
 DrawEventRecorder *Factory::mRecorder;
 
 mozilla::gfx::Config* Factory::sConfig = nullptr;
 
 void
 Factory::Init(const Config& aConfig)