Short term fix for SkClipStack unique key issue
The unique key issue is competition between the installation of
unique keys on GrGpuResources as DDLs are replayed and the asynchronous
removal of those same unique keys via the message bus. This CL
remedies the situation by making the invalidation of the clip-stack's
unique keys happen immediately and synchronously in the DDL recorder
thread.
Change-Id: Ib4923fe40a1cacbc55225f81bd4b7dd896b13f77
Reviewed-on: https://skia-review.googlesource.com/c/179721
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
diff --git a/src/core/SkClipStack.cpp b/src/core/SkClipStack.cpp
index 334ef1f..7cbdf07 100644
--- a/src/core/SkClipStack.cpp
+++ b/src/core/SkClipStack.cpp
@@ -13,6 +13,10 @@
#include <atomic>
#include <new>
+#if SK_SUPPORT_GPU
+#include "GrProxyProvider.h"
+#endif
+
SkClipStack::Element::Element(const Element& that) {
switch (that.getDeviceSpaceType()) {
case DeviceSpaceType::kEmpty:
@@ -39,6 +43,15 @@
fGenID = that.fGenID;
}
+SkClipStack::Element::~Element() {
+#if SK_SUPPORT_GPU
+ for (int i = 0; i < fKeysToInvalidate.count(); ++i) {
+ fProxyProvider->processInvalidUniqueKey(fKeysToInvalidate[i], nullptr,
+ GrProxyProvider::InvalidateGPUResource::kYes);
+ }
+#endif
+}
+
bool SkClipStack::Element::operator== (const Element& element) const {
if (this == &element) {
return true;
diff --git a/src/core/SkClipStack.h b/src/core/SkClipStack.h
index e96a47f..87288d8 100644
--- a/src/core/SkClipStack.h
+++ b/src/core/SkClipStack.h
@@ -19,6 +19,8 @@
#include "SkTLazy.h"
#if SK_SUPPORT_GPU
+class GrProxyProvider;
+
#include "GrResourceKey.h"
#endif
@@ -81,13 +83,7 @@
this->initPath(0, path, m, op, doAA);
}
- ~Element() {
-#if SK_SUPPORT_GPU
- for (int i = 0; i < fMessages.count(); ++i) {
- SkMessageBus<GrUniqueKeyInvalidatedMessage>::Post(*fMessages[i]);
- }
-#endif
- }
+ ~Element();
bool operator== (const Element& element) const;
bool operator!= (const Element& element) const { return !(*this == element); }
@@ -181,9 +177,16 @@
* This is used to purge any GPU resource cache items that become unreachable when
* the element is destroyed because their key is based on this element's gen ID.
*/
- void addResourceInvalidationMessage(
- std::unique_ptr<GrUniqueKeyInvalidatedMessage> msg) const {
- fMessages.emplace_back(std::move(msg));
+ void addResourceInvalidationMessage(GrProxyProvider* proxyProvider,
+ const GrUniqueKey& key) const {
+ SkASSERT(proxyProvider);
+
+ if (!fProxyProvider) {
+ fProxyProvider = proxyProvider;
+ }
+ SkASSERT(fProxyProvider == proxyProvider);
+
+ fKeysToInvalidate.push_back(key);
}
#endif
@@ -216,7 +219,8 @@
uint32_t fGenID;
#if SK_SUPPORT_GPU
- mutable SkTArray<std::unique_ptr<GrUniqueKeyInvalidatedMessage>> fMessages;
+ mutable GrProxyProvider* fProxyProvider = nullptr;
+ mutable SkTArray<GrUniqueKey> fKeysToInvalidate;
#endif
Element(int saveCount) {
this->initCommon(saveCount, kReplace_SkClipOp, false);
@@ -515,3 +519,4 @@
};
#endif
+
diff --git a/src/gpu/GrClipStackClip.cpp b/src/gpu/GrClipStackClip.cpp
index ca5043d..b62f9a1 100644
--- a/src/gpu/GrClipStackClip.cpp
+++ b/src/gpu/GrClipStackClip.cpp
@@ -316,15 +316,15 @@
builder[3] = numAnalyticFPs;
}
-static void add_invalidate_on_pop_message(const SkClipStack& stack, uint32_t clipGenID,
- const GrUniqueKey& clipMaskKey,
- uint32_t contextUniqueID) {
+static void add_invalidate_on_pop_message(GrContext* context,
+ const SkClipStack& stack, uint32_t clipGenID,
+ const GrUniqueKey& clipMaskKey) {
+ GrProxyProvider* proxyProvider = context->contextPriv().proxyProvider();
+
SkClipStack::Iter iter(stack, SkClipStack::Iter::kTop_IterStart);
while (const Element* element = iter.prev()) {
if (element->getGenID() == clipGenID) {
- std::unique_ptr<GrUniqueKeyInvalidatedMessage> msg(
- new GrUniqueKeyInvalidatedMessage(clipMaskKey, contextUniqueID));
- element->addResourceInvalidationMessage(std::move(msg));
+ element->addResourceInvalidationMessage(proxyProvider, clipMaskKey);
return;
}
}
@@ -371,7 +371,7 @@
SkASSERT(result->origin() == kTopLeft_GrSurfaceOrigin);
proxyProvider->assignUniqueKeyToProxy(key, result.get());
- add_invalidate_on_pop_message(*fStack, reducedClip.maskGenID(), key, context->uniqueID());
+ add_invalidate_on_pop_message(context, *fStack, reducedClip.maskGenID(), key);
return result;
}
@@ -517,6 +517,6 @@
SkASSERT(proxy->origin() == kTopLeft_GrSurfaceOrigin);
proxyProvider->assignUniqueKeyToProxy(key, proxy.get());
- add_invalidate_on_pop_message(*fStack, reducedClip.maskGenID(), key, context->uniqueID());
+ add_invalidate_on_pop_message(context, *fStack, reducedClip.maskGenID(), key);
return proxy;
}
diff --git a/src/gpu/GrProxyProvider.cpp b/src/gpu/GrProxyProvider.cpp
index c6a0990..0e9616a 100644
--- a/src/gpu/GrProxyProvider.cpp
+++ b/src/gpu/GrProxyProvider.cpp
@@ -110,7 +110,7 @@
return;
}
- this->processInvalidProxyUniqueKey(key, proxy, true);
+ this->processInvalidUniqueKey(key, proxy, GrProxyProvider::InvalidateGPUResource::kYes);
}
sk_sp<GrTextureProxy> GrProxyProvider::findProxyByUniqueKey(const GrUniqueKey& key,
@@ -653,28 +653,38 @@
proxy->worstCaseHeight() == proxy->height());
}
-void GrProxyProvider::processInvalidProxyUniqueKey(const GrUniqueKey& key) {
+void GrProxyProvider::processInvalidUniqueKey(const GrUniqueKey& key, GrTextureProxy* proxy,
+ InvalidateGPUResource invalidateGPUResource) {
+ if (!proxy) {
+ proxy = fUniquelyKeyedProxies.find(key);
+ }
+
// Note: this method is called for the whole variety of GrGpuResources so often 'key'
// will not be in 'fUniquelyKeyedProxies'.
- GrTextureProxy* proxy = fUniquelyKeyedProxies.find(key);
if (proxy) {
- this->processInvalidProxyUniqueKey(key, proxy, false);
+ SkASSERT(proxy->getUniqueKey().isValid());
+ SkASSERT(proxy->getUniqueKey() == key);
+
+ fUniquelyKeyedProxies.remove(key);
+ proxy->cacheAccess().clearUniqueKey();
}
-}
-void GrProxyProvider::processInvalidProxyUniqueKey(const GrUniqueKey& key, GrTextureProxy* proxy,
- bool invalidateSurface) {
- SkASSERT(proxy);
- SkASSERT(proxy->getUniqueKey().isValid());
- SkASSERT(proxy->getUniqueKey() == key);
+ if (InvalidateGPUResource::kNo == invalidateGPUResource) {
+ return;
+ }
- fUniquelyKeyedProxies.remove(key);
- proxy->cacheAccess().clearUniqueKey();
-
- if (invalidateSurface && proxy->isInstantiated()) {
+ if (proxy && proxy->isInstantiated()) {
GrSurface* surface = proxy->peekSurface();
if (surface) {
surface->resourcePriv().removeUniqueKey();
+ return;
+ }
+ }
+
+ if (fResourceProvider) {
+ sk_sp<GrGpuResource> resource = fResourceProvider->findByUniqueKey<GrGpuResource>(key);
+ if (resource) {
+ resource->resourcePriv().removeUniqueKey();
}
}
}
@@ -693,7 +703,8 @@
for (UniquelyKeyedProxyHash::Iter iter(&fUniquelyKeyedProxies); !iter.done(); ++iter) {
GrTextureProxy& tmp = *iter;
- this->processInvalidProxyUniqueKey(tmp.getUniqueKey(), &tmp, false);
+ this->processInvalidUniqueKey(tmp.getUniqueKey(), &tmp,
+ GrProxyProvider::InvalidateGPUResource::kNo);
}
SkASSERT(!fUniquelyKeyedProxies.count());
}
diff --git a/src/gpu/GrProxyProvider.h b/src/gpu/GrProxyProvider.h
index 1aec328..8cdaab5 100644
--- a/src/gpu/GrProxyProvider.h
+++ b/src/gpu/GrProxyProvider.h
@@ -189,21 +189,23 @@
// determine if it is going to need a texture domain or a full clear.
static bool IsFunctionallyExact(GrSurfaceProxy* proxy);
- /**
- * Either the proxy attached to the unique key is being deleted (in which case we
- * don't want it cluttering up the hash table) or the client has indicated that
- * it will never refer to the unique key again. In either case, remove the key
- * from the hash table.
- * Note: this does not, by itself, alter unique key attached to the underlying GrTexture.
- */
- void processInvalidProxyUniqueKey(const GrUniqueKey&);
+ enum class InvalidateGPUResource : bool { kNo = false, kYes = true };
- /**
- * Same as above, but you must pass in a GrTextureProxy to save having to search for it. The
- * GrUniqueKey of the proxy must be valid and it must match the passed in key. This function
- * also gives the option to invalidate the GrUniqueKey on the underlying GrTexture.
+ /*
+ * This method ensures that, if a proxy w/ the supplied unique key exists, it is removed from
+ * the proxy provider's map and its unique key is removed. If 'invalidateSurface' is true, it
+ * will independently ensure that the unique key is removed from any GrGpuResources that may
+ * have it.
+ *
+ * If 'proxy' is provided (as an optimization to stop re-looking it up), its unique key must be
+ * valid and match the provided unique key.
+ *
+ * This method is called if either the proxy attached to the unique key is being deleted
+ * (in which case we don't want it cluttering up the hash table) or the client has indicated
+ * that it will never refer to the unique key again.
*/
- void processInvalidProxyUniqueKey(const GrUniqueKey&, GrTextureProxy*, bool invalidateSurface);
+ void processInvalidUniqueKey(const GrUniqueKey&, GrTextureProxy*,
+ InvalidateGPUResource invalidateGPUResource);
uint32_t contextUniqueID() const { return fContextUniqueID; }
const GrCaps* caps() const { return fCaps.get(); }
diff --git a/src/gpu/GrResourceCache.cpp b/src/gpu/GrResourceCache.cpp
index dceb87e..53dfe9b 100644
--- a/src/gpu/GrResourceCache.cpp
+++ b/src/gpu/GrResourceCache.cpp
@@ -484,7 +484,13 @@
SkTArray<GrUniqueKeyInvalidatedMessage> invalidKeyMsgs;
fInvalidUniqueKeyInbox.poll(&invalidKeyMsgs);
if (invalidKeyMsgs.count()) {
- this->processInvalidUniqueKeys(invalidKeyMsgs);
+ SkASSERT(fProxyProvider);
+
+ for (int i = 0; i < invalidKeyMsgs.count(); ++i) {
+ fProxyProvider->processInvalidUniqueKey(invalidKeyMsgs[i].key(), nullptr,
+ GrProxyProvider::InvalidateGPUResource::kYes);
+ SkASSERT(!this->findAndRefUniqueResource(invalidKeyMsgs[i].key()));
+ }
}
this->processFreedGpuResources();
@@ -592,21 +598,6 @@
}
}
-void GrResourceCache::processInvalidUniqueKeys(
- const SkTArray<GrUniqueKeyInvalidatedMessage>& msgs) {
- SkASSERT(fProxyProvider); // better have called setProxyProvider
-
- for (int i = 0; i < msgs.count(); ++i) {
- fProxyProvider->processInvalidProxyUniqueKey(msgs[i].key());
-
- GrGpuResource* resource = this->findAndRefUniqueResource(msgs[i].key());
- if (resource) {
- resource->resourcePriv().removeUniqueKey();
- resource->unref(); // If this resource is now purgeable, the cache will be notified.
- }
- }
-}
-
void GrResourceCache::insertCrossContextGpuResource(GrGpuResource* resource) {
resource->ref();
SkASSERT(!fResourcesWaitingForFreeMsg.contains(resource));
diff --git a/src/gpu/GrResourceCache.h b/src/gpu/GrResourceCache.h
index 50f26bf..7730ebd 100644
--- a/src/gpu/GrResourceCache.h
+++ b/src/gpu/GrResourceCache.h
@@ -264,7 +264,6 @@
void refAndMakeResourceMRU(GrGpuResource*);
/// @}
- void processInvalidUniqueKeys(const SkTArray<GrUniqueKeyInvalidatedMessage>&);
void processFreedGpuResources();
void addToNonpurgeableArray(GrGpuResource*);
void removeFromNonpurgeableArray(GrGpuResource*);
diff --git a/src/gpu/GrTextureProxy.cpp b/src/gpu/GrTextureProxy.cpp
index b732818..b15b0b2 100644
--- a/src/gpu/GrTextureProxy.cpp
+++ b/src/gpu/GrTextureProxy.cpp
@@ -69,7 +69,8 @@
// proxy provider has gone away. In that case there is noone to send the invalid key
// message to (Note: in this case we don't want to remove its cached resource).
if (fUniqueKey.isValid() && fProxyProvider) {
- fProxyProvider->processInvalidProxyUniqueKey(fUniqueKey, this, false);
+ fProxyProvider->processInvalidUniqueKey(fUniqueKey, this,
+ GrProxyProvider::InvalidateGPUResource::kNo);
} else {
SkASSERT(!fProxyProvider);
}