From 64a3f8fcb74929e34acf1831f58cabf2e435042b Mon Sep 17 00:00:00 2001 From: Brian Salomon Date: Wed, 4 Mar 2020 03:39:52 +0000 Subject: [PATCH] Revert "Use spin lock in SkIDChangeListener" This reverts commit a624a534edb23eafe6a6caf597301c11e2abd21b. 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 > Commit-Queue: Brian Salomon TBR=bsalomon@google.com,brianosman@google.com 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 Commit-Queue: Brian Salomon --- BUILD.gn | 1 - include/private/SkIDChangeListener.h | 10 ++--- src/core/SkIDChangeListener.cpp | 62 ++++++++++++++++++---------- src/core/SkPathRef.cpp | 8 +++- src/core/SkPixelRef.cpp | 8 ++-- src/image/SkImage_Lazy.cpp | 3 +- 6 files changed, 59 insertions(+), 33 deletions(-) diff --git a/BUILD.gn b/BUILD.gn index 841f263aea..584683dbe9 100644 --- a/BUILD.gn +++ b/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", diff --git a/include/private/SkIDChangeListener.h b/include/private/SkIDChangeListener.h index 15b9bc2427..7cf04129fb 100644 --- a/include/private/SkIDChangeListener.h +++ b/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 @@ -49,7 +49,7 @@ public: * Add a new listener to the list. It must not already be deregistered. Also clears out * previously deregistered listeners. */ - void add(sk_sp listener); + void add(sk_sp listener, bool singleThreaded = false); /** * The number of registered listeners (including deregisterd listeners that are yet-to-be @@ -58,13 +58,13 @@ public: 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 fListeners; // pointers are reffed }; diff --git a/src/core/SkIDChangeListener.cpp b/src/core/SkIDChangeListener.cpp index 395f26389e..954b4688ff 100644 --- a/src/core/SkIDChangeListener.cpp +++ b/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 listener) { +void List::add(sk_sp 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(); + } } diff --git a/src/core/SkPathRef.cpp b/src/core/SkPathRef.cpp index d8856c34ff..fd1ab55d0b 100644 --- a/src/core/SkPathRef.cpp +++ b/src/core/SkPathRef.cpp @@ -479,13 +479,17 @@ void SkPathRef::addGenIDChangeListener(sk_sp 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(); diff --git a/src/core/SkPixelRef.cpp b/src/core/SkPixelRef.cpp index dcd1323a3d..3174441032 100644 --- a/src/core/SkPixelRef.cpp +++ b/src/core/SkPixelRef.cpp @@ -79,21 +79,23 @@ void SkPixelRef::addGenIDChangeListener(sk_sp 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); } } diff --git a/src/image/SkImage_Lazy.cpp b/src/image/SkImage_Lazy.cpp index 9c4868a070..b46c77a345 100644 --- a/src/image/SkImage_Lazy.cpp +++ b/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 listener) const { - fUniqueIDListeners.add(std::move(listener)); + bool singleThreaded = this->unique(); + fUniqueIDListeners.add(std::move(listener), singleThreaded); } #endif