Revert "Use spin lock in SkIDChangeListener"

This reverts commit a624a53.

Reason for revert: Bad perf

Original change's description:
> Use spin lock in SkIDChangeListener
> 
> This lock should almost never be contested. This simplifies things and
> also avoid mutex locks when one thread has multiple refs on a path,
> image, ...
> 
> Change-Id: I909b490363cb9e81b38ed9edd04272133fb2692b
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/274676
> Reviewed-by: Brian Osman <[email protected]>
> Commit-Queue: Brian Salomon <[email protected]>

[email protected],[email protected]

Change-Id: I45ec3241906429e4d0783b68ebe6398079b73d36
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/274956
Reviewed-by: Brian Salomon <[email protected]>
Commit-Queue: Brian Salomon <[email protected]>

Author: bsalomon

BUILD.gn

@@ -1107,7 +1107,6 @@ static_library("pathkit") {
     "src/core/SkRRect.cpp",
     "src/core/SkRect.cpp",
     "src/core/SkSemaphore.cpp",
-    "src/core/SkSpinlock.cpp",
     "src/core/SkStream.cpp",
     "src/core/SkString.cpp",
     "src/core/SkStringUtils.cpp",

include/private/SkIDChangeListener.h

@@ -9,7 +9,7 @@
 #define SkIDChangeListener_DEFINED
 
 #include "include/core/SkRefCnt.h"
-#include "include/private/SkSpinlock.h"
+#include "include/private/SkMutex.h"
 #include "include/private/SkTDArray.h"
 
 #include <atomic>
@@ -49,7 +49,7 @@ class SkIDChangeListener : public SkRefCnt {
          * Add a new listener to the list. It must not already be deregistered. Also clears out
          * previously deregistered listeners.
          */
-        void add(sk_sp<SkIDChangeListener> listener);
+        void add(sk_sp<SkIDChangeListener> listener, bool singleThreaded = false);
 
         /**
          * The number of registered listeners (including deregisterd listeners that are yet-to-be
@@ -58,13 +58,13 @@ class SkIDChangeListener : public SkRefCnt {
         int count();
 
         /** Calls changed() on all listeners that haven't been deregistered and resets the list. */
-        void changed();
+        void changed(bool singleThreaded = false);
 
         /** Resets without calling changed() on the listeners. */
-        void reset();
+        void reset(bool singleThreaded = false);
 
     private:
-        SkSpinlock fSpinlock;
+        SkMutex fMutex;
         SkTDArray<SkIDChangeListener*> fListeners;  // pointers are reffed
     };
 

src/core/SkIDChangeListener.cpp

@@ -24,7 +24,7 @@ using List = SkIDChangeListener::List;
 List::List() = default;
 
 List::~List() {
-    // We don't need the lock. No other thread should have this list while it's being
+    // We don't need the mutex. No other thread should have this list while it's being
     // destroyed.
     for (int i = 0; i < fListeners.count(); ++i) {
         if (!fListeners[i]->shouldDeregister()) {
@@ -34,41 +34,61 @@ List::~List() {
     }
 }
 
-void List::add(sk_sp<SkIDChangeListener> listener) {
+void List::add(sk_sp<SkIDChangeListener> listener, bool singleThreaded) {
     if (!listener) {
         return;
     }
     SkASSERT(!listener->shouldDeregister());
 
-    SkAutoSpinlock lock(fSpinlock);
-    // Clean out any stale listeners before we append the new one.
-    for (int i = 0; i < fListeners.count(); ++i) {
-        if (fListeners[i]->shouldDeregister()) {
-            fListeners[i]->unref();
-            fListeners.removeShuffle(i--);  // No need to preserve the order after i.
+    auto add = [&] {
+        // Clean out any stale listeners before we append the new one.
+        for (int i = 0; i < fListeners.count(); ++i) {
+            if (fListeners[i]->shouldDeregister()) {
+                fListeners[i]->unref();
+                fListeners.removeShuffle(i--);  // No need to preserve the order after i.
+            }
         }
+        *fListeners.append() = listener.release();
+    };
+
+    if (singleThreaded) {
+        add();
+    } else {
+        SkAutoMutexExclusive lock(fMutex);
+        add();
     }
-    *fListeners.append() = listener.release();
 }
 
 int List::count() {
-    SkAutoSpinlock lock(fSpinlock);
+    SkAutoMutexExclusive lock(fMutex);
     return fListeners.count();
 }
 
-void List::changed() {
-    SkAutoSpinlock lock(fSpinlock);
-    for (SkIDChangeListener* listener : fListeners) {
-        if (!listener->shouldDeregister()) {
-            listener->changed();
+void List::changed(bool singleThreaded) {
+    auto visit = [this]() {
+        for (SkIDChangeListener* listener : fListeners) {
+            if (!listener->shouldDeregister()) {
+                listener->changed();
+            }
+            // Listeners get at most one shot, so whether these triggered or not, blow them away.
+            listener->unref();
         }
-        // Listeners get at most one shot, so whether these triggered or not, blow them away.
-        listener->unref();
+        fListeners.reset();
+    };
+
+    if (singleThreaded) {
+        visit();
+    } else {
+        SkAutoMutexExclusive lock(fMutex);
+        visit();
     }
-    fListeners.reset();
 }
 
-void List::reset() {
-    SkAutoSpinlock lock(fSpinlock);
-    fListeners.unrefAll();
+void List::reset(bool singleThreaded) {
+    if (singleThreaded) {
+        fListeners.unrefAll();
+    } else {
+        SkAutoMutexExclusive lock(fMutex);
+        fListeners.unrefAll();
+    }
 }

src/core/SkPathRef.cpp

@@ -479,13 +479,17 @@ void SkPathRef::addGenIDChangeListener(sk_sp<SkIDChangeListener> listener) {
     if (this == gEmpty) {
         return;
     }
-    fGenIDChangeListeners.add(std::move(listener));
+    bool singleThreaded = this->unique();
+    fGenIDChangeListeners.add(std::move(listener), singleThreaded);
 }
 
 int SkPathRef::genIDChangeListenerCount() { return fGenIDChangeListeners.count(); }
 
 // we need to be called *before* the genID gets changed or zerod
-void SkPathRef::callGenIDChangeListeners() { fGenIDChangeListeners.changed(); }
+void SkPathRef::callGenIDChangeListeners() {
+    bool singleThreaded = this->unique();
+    fGenIDChangeListeners.changed(singleThreaded);
+}
 
 SkRRect SkPathRef::getRRect() const {
     const SkRect& bounds = this->getBounds();

src/core/SkPixelRef.cpp

@@ -79,21 +79,23 @@ void SkPixelRef::addGenIDChangeListener(sk_sp<SkIDChangeListener> listener) {
         return;
     }
     SkASSERT(!listener->shouldDeregister());
-    fGenIDChangeListeners.add(std::move(listener));
+    bool singleThreaded = this->unique();
+    fGenIDChangeListeners.add(std::move(listener), singleThreaded);
 }
 
 // we need to be called *before* the genID gets changed or zerod
 void SkPixelRef::callGenIDChangeListeners() {
+    bool singleThreaded = this->unique();
     // We don't invalidate ourselves if we think another SkPixelRef is sharing our genID.
     if (this->genIDIsUnique()) {
-        fGenIDChangeListeners.changed();
+        fGenIDChangeListeners.changed(singleThreaded);
         if (fAddedToCache.exchange(false)) {
             SkNotifyBitmapGenIDIsStale(this->getGenerationID());
         }
     } else {
         // Listeners get at most one shot, so even though these weren't triggered or not, blow them
         // away.
-        fGenIDChangeListeners.reset();
+        fGenIDChangeListeners.reset(singleThreaded);
     }
 }
 

src/image/SkImage_Lazy.cpp

@@ -514,7 +514,8 @@ GrColorType SkImage_Lazy::colorTypeOfLockTextureProxy(const GrCaps* caps) const
 
 #if SK_SUPPORT_GPU
 void SkImage_Lazy::addUniqueIDListener(sk_sp<SkIDChangeListener> listener) const {
-    fUniqueIDListeners.add(std::move(listener));
+    bool singleThreaded = this->unique();
+    fUniqueIDListeners.add(std::move(listener), singleThreaded);
 }
 #endif