Change the meaning of GrBudgetedType::kUnbudgetedUncacheable.
kUnbudgetedCacheable now means that the resource is never purged
until its unique key is removed.
This fixes an issue where a cached texture for a promise image
might get purged by cache pressure. This in turn could cause
Skia to call the promise image's Fulfill proc multiple times with
no intervening Release calls. The balancing Release calls would
occur, but the policy is that each Fulfill should be balanced by
Release *before* another Fulfill.
Update/add unit tests.
Bug: chromium:922851
Change-Id: I6411e413b3104721ca4bb6e7f07b3b73d14cbcf9
Reviewed-on: https://skia-review.googlesource.com/c/186361
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 449fe60..d122a4d 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -261,7 +261,8 @@
bool GrContext::abandoned() const {
ASSERT_SINGLE_OWNER
- return fDrawingManager->wasAbandoned();
+ // If called from ~GrContext(), the drawing manager may already be gone.
+ return !fDrawingManager || fDrawingManager->wasAbandoned();
}
void GrContext::releaseResourcesAndAbandonContext() {
diff --git a/src/gpu/GrGpuResource.cpp b/src/gpu/GrGpuResource.cpp
index c9224be..02cc617 100644
--- a/src/gpu/GrGpuResource.cpp
+++ b/src/gpu/GrGpuResource.cpp
@@ -93,6 +93,17 @@
this->setMemoryBacking(traceMemoryDump, resourceName);
}
+bool GrGpuResource::isPurgeable() const {
+ // Resources in the kUnbudgetedCacheable state are never purgeable when they have a unique
+ // key. The key must be removed/invalidated to make them purgeable.
+ return !this->hasRefOrPendingIO() &&
+ !(fBudgetedType == GrBudgetedType::kUnbudgetedCacheable && fUniqueKey.isValid());
+}
+
+bool GrGpuResource::hasRefOrPendingIO() const {
+ return this->internalHasRef() || this->internalHasPendingIO();
+}
+
SkString GrGpuResource::getResourceName() const {
// Dump resource as "skia/gpu_resources/resource_#".
SkString resourceName("skia/gpu_resources/resource_");
@@ -145,6 +156,8 @@
}
void GrGpuResource::notifyAllCntsAreZero(CntType lastCntTypeToReachZero) const {
+ GrGpuResource* mutableThis = const_cast<GrGpuResource*>(this);
+ mutableThis->removedLastRefOrPendingIO();
if (this->wasDestroyed()) {
// We've already been removed from the cache. Goodbye cruel world!
delete this;
@@ -154,7 +167,6 @@
// We should have already handled this fully in notifyRefCntIsZero().
SkASSERT(kRef_CntType != lastCntTypeToReachZero);
- GrGpuResource* mutableThis = const_cast<GrGpuResource*>(this);
static const uint32_t kFlag =
GrResourceCache::ResourceAccess::kAllCntsReachedZero_RefNotificationFlag;
get_resource_cache(fGpu)->resourceAccess().notifyCntReachedZero(mutableThis, kFlag);
@@ -170,6 +182,7 @@
uint32_t flags = GrResourceCache::ResourceAccess::kRefCntReachedZero_RefNotificationFlag;
if (!this->internalHasPendingIO()) {
flags |= GrResourceCache::ResourceAccess::kAllCntsReachedZero_RefNotificationFlag;
+ mutableThis->removedLastRefOrPendingIO();
}
get_resource_cache(fGpu)->resourceAccess().notifyCntReachedZero(mutableThis, flags);
diff --git a/src/gpu/GrGpuResourceCacheAccess.h b/src/gpu/GrGpuResourceCacheAccess.h
index 3765c6e..e80b9b5 100644
--- a/src/gpu/GrGpuResourceCacheAccess.h
+++ b/src/gpu/GrGpuResourceCacheAccess.h
@@ -30,17 +30,11 @@
}
/**
- * Called by GrResourceCache when a resource becomes purgeable regardless of whether the cache
- * has decided to keep the resource ot purge it immediately.
- */
- void becamePurgeable() { fResource->becamePurgeable(); }
-
- /**
* Called by the cache to delete the resource under normal circumstances.
*/
void release() {
fResource->release();
- if (fResource->isPurgeable()) {
+ if (!fResource->hasRefOrPendingIO()) {
delete fResource;
}
}
@@ -50,7 +44,7 @@
*/
void abandon() {
fResource->abandon();
- if (fResource->isPurgeable()) {
+ if (!fResource->hasRefOrPendingIO()) {
delete fResource;
}
}
diff --git a/src/gpu/GrGpuResourcePriv.h b/src/gpu/GrGpuResourcePriv.h
index 75b3531..30f686c 100644
--- a/src/gpu/GrGpuResourcePriv.h
+++ b/src/gpu/GrGpuResourcePriv.h
@@ -72,6 +72,8 @@
bool isPurgeable() const { return fResource->isPurgeable(); }
+ bool hasRefOrPendingIO() const { return fResource->hasRefOrPendingIO(); }
+
protected:
ResourcePriv(GrGpuResource* resource) : fResource(resource) { }
ResourcePriv(const ResourcePriv& that) : fResource(that.fResource) {}
diff --git a/src/gpu/GrProxyProvider.h b/src/gpu/GrProxyProvider.h
index f84cdfc..f093416 100644
--- a/src/gpu/GrProxyProvider.h
+++ b/src/gpu/GrProxyProvider.h
@@ -251,6 +251,7 @@
*/
sk_sp<GrTextureProxy> testingOnly_createInstantiatedProxy(const GrSurfaceDesc&, GrSurfaceOrigin,
SkBackingFit, SkBudgeted);
+ sk_sp<GrTextureProxy> testingOnly_createWrapped(sk_sp<GrTexture>, GrSurfaceOrigin);
private:
friend class GrAHardwareBufferImageGenerator; // for createWrapped
diff --git a/src/gpu/GrResourceCache.cpp b/src/gpu/GrResourceCache.cpp
index 634dac7..dfb1df4 100644
--- a/src/gpu/GrResourceCache.cpp
+++ b/src/gpu/GrResourceCache.cpp
@@ -187,9 +187,6 @@
while (fNonpurgeableResources.count()) {
GrGpuResource* back = *(fNonpurgeableResources.end() - 1);
SkASSERT(!back->wasDestroyed());
- // If these resources we're relying on a purgeable notification to release something, notify
- // them now. They aren't in the purgeable queue but they're getting purged anyway.
- back->cacheAccess().becamePurgeable();
back->cacheAccess().abandon();
}
@@ -230,9 +227,6 @@
while (fNonpurgeableResources.count()) {
GrGpuResource* back = *(fNonpurgeableResources.end() - 1);
SkASSERT(!back->wasDestroyed());
- // If these resources we're relying on a purgeable notification to release something, notify
- // them now. They aren't in the purgeable queue but they're getting purged anyway.
- back->cacheAccess().becamePurgeable();
back->cacheAccess().release();
}
@@ -319,11 +313,14 @@
fUniqueHash.remove(resource->getUniqueKey());
}
resource->cacheAccess().removeUniqueKey();
-
if (resource->resourcePriv().getScratchKey().isValid()) {
fScratchMap.insert(resource->resourcePriv().getScratchKey(), resource);
}
+ // Removing a unique key from a kUnbudgetedCacheable resource would make the resource
+ // require purging. However, the resource must be ref'ed to get here and therefore can't
+ // be purgeable. We'll purge it when the refs reach zero.
+ SkASSERT(!resource->resourcePriv().isPurgeable());
this->validate();
}
@@ -340,7 +337,8 @@
old->resourcePriv().isPurgeable()) {
old->cacheAccess().release();
} else {
- this->removeUniqueKey(old);
+ // removeUniqueKey expects an external owner of the resource.
+ this->removeUniqueKey(sk_ref_sp(old).get());
}
}
SkASSERT(nullptr == fUniqueHash.find(newKey));
@@ -412,7 +410,11 @@
return;
}
- SkASSERT(resource->resourcePriv().isPurgeable());
+ if (!resource->resourcePriv().isPurgeable()) {
+ this->validate();
+ return;
+ }
+
this->removeFromNonpurgeableArray(resource);
fPurgeableQueue.insert(resource);
resource->cacheAccess().setTimeWhenResourceBecomePurgeable();
@@ -420,35 +422,33 @@
bool hasUniqueKey = resource->getUniqueKey().isValid();
- {
- SkScopeExit notifyPurgeable([resource] { resource->cacheAccess().becamePurgeable(); });
+ GrBudgetedType budgetedType = resource->resourcePriv().budgetedType();
- auto budgetedType = resource->resourcePriv().budgetedType();
- if (budgetedType == GrBudgetedType::kBudgeted) {
- // Purge the resource immediately if we're over budget
- // Also purge if the resource has neither a valid scratch key nor a unique key.
- bool hasKey = resource->resourcePriv().getScratchKey().isValid() || hasUniqueKey;
- if (!this->overBudget() && hasKey) {
+ if (budgetedType == GrBudgetedType::kBudgeted) {
+ // Purge the resource immediately if we're over budget
+ // Also purge if the resource has neither a valid scratch key nor a unique key.
+ bool hasKey = resource->resourcePriv().getScratchKey().isValid() || hasUniqueKey;
+ if (!this->overBudget() && hasKey) {
+ return;
+ }
+ } else {
+ // We keep unbudgeted resources with a unique key in the purgeable queue of the cache so
+ // they can be reused again by the image connected to the unique key.
+ if (hasUniqueKey && budgetedType == GrBudgetedType::kUnbudgetedCacheable) {
+ return;
+ }
+ // Check whether this resource could still be used as a scratch resource.
+ if (!resource->resourcePriv().refsWrappedObjects() &&
+ resource->resourcePriv().getScratchKey().isValid()) {
+ // We won't purge an existing resource to make room for this one.
+ if (fBudgetedCount < fMaxCount &&
+ fBudgetedBytes + resource->gpuMemorySize() <= fMaxBytes) {
+ resource->resourcePriv().makeBudgeted();
return;
}
- } else {
- // We keep unbudgeted resources with a unique key in the purgable queue of the cache so
- // they can be reused again by the image connected to the unique key.
- if (hasUniqueKey && budgetedType == GrBudgetedType::kUnbudgetedCacheable) {
- return;
- }
- // Check whether this resource could still be used as a scratch resource.
- if (!resource->resourcePriv().refsWrappedObjects() &&
- resource->resourcePriv().getScratchKey().isValid()) {
- // We won't purge an existing resource to make room for this one.
- if (fBudgetedCount < fMaxCount &&
- fBudgetedBytes + resource->gpuMemorySize() <= fMaxBytes) {
- resource->resourcePriv().makeBudgeted();
- return;
- }
- }
}
}
+
SkDEBUGCODE(int beforeCount = this->getResourceCount();)
resource->cacheAccess().release();
// We should at least free this resource, perhaps dependent resources as well.
@@ -462,8 +462,12 @@
SkASSERT(this->isInCache(resource));
size_t size = resource->gpuMemorySize();
-
- if (GrBudgetedType::kBudgeted == resource->resourcePriv().budgetedType()) {
+ // Changing from BudgetedType::kUnbudgetedCacheable to another budgeted type could make
+ // resource become purgeable. However, we should never allow that transition. Wrapped
+ // resources are the only resources that can be in that state and they aren't allowed to
+ // transition from one budgeted state to another.
+ SkDEBUGCODE(bool wasPurgeable = resource->resourcePriv().isPurgeable());
+ if (resource->resourcePriv().budgetedType() == GrBudgetedType::kBudgeted) {
++fBudgetedCount;
fBudgetedBytes += size;
#if GR_CACHE_STATS
@@ -472,9 +476,11 @@
#endif
this->purgeAsNeeded();
} else {
+ SkASSERT(resource->resourcePriv().budgetedType() != GrBudgetedType::kUnbudgetedCacheable);
--fBudgetedCount;
fBudgetedBytes -= size;
}
+ SkASSERT(wasPurgeable == resource->resourcePriv().isPurgeable());
TRACE_COUNTER2("skia.gpu.cache", "skia budget", "used",
fBudgetedBytes, "free", fMaxBytes - fBudgetedBytes);
diff --git a/src/gpu/gl/GrGLTexture.h b/src/gpu/gl/GrGLTexture.h
index e8ca83c..9383f11 100644
--- a/src/gpu/gl/GrGLTexture.h
+++ b/src/gpu/gl/GrGLTexture.h
@@ -137,7 +137,7 @@
fReleaseHelper.reset();
}
- void becamePurgeable() override {
+ void removedLastRefOrPendingIO() override {
if (fIdleProc) {
fIdleProc(fIdleProcContext);
fIdleProc = nullptr;
diff --git a/src/gpu/mock/GrMockTexture.h b/src/gpu/mock/GrMockTexture.h
index 26741be..c44809a 100644
--- a/src/gpu/mock/GrMockTexture.h
+++ b/src/gpu/mock/GrMockTexture.h
@@ -77,7 +77,7 @@
// protected so that GrMockTextureRenderTarget can call this to avoid "inheritance via
// dominance" warning.
- void becamePurgeable() override {
+ void removedLastRefOrPendingIO() override {
if (fIdleProc) {
fIdleProc(fIdleProcContext);
fIdleProc = nullptr;
@@ -196,7 +196,7 @@
}
// We implement this to avoid the inheritance via dominance warning.
- void becamePurgeable() override { GrMockTexture::becamePurgeable(); }
+ void removedLastRefOrPendingIO() override { GrMockTexture::removedLastRefOrPendingIO(); }
size_t onGpuMemorySize() const override {
int numColorSamples = this->numColorSamples();
diff --git a/src/gpu/mtl/GrMtlTexture.h b/src/gpu/mtl/GrMtlTexture.h
index 87599ea..a01bfa4 100644
--- a/src/gpu/mtl/GrMtlTexture.h
+++ b/src/gpu/mtl/GrMtlTexture.h
@@ -76,7 +76,7 @@
fReleaseHelper.reset();
}
- void becamePurgeable() override {
+ void removedLastRefOrPendingIO() override {
if (fIdleProc) {
fIdleProc(fIdleProcContext);
fIdleProc = nullptr;
diff --git a/src/gpu/vk/GrVkImage.cpp b/src/gpu/vk/GrVkImage.cpp
index ead8d8d..755ccab 100644
--- a/src/gpu/vk/GrVkImage.cpp
+++ b/src/gpu/vk/GrVkImage.cpp
@@ -262,7 +262,7 @@
if (--fNumCommandBufferOwners || !fIdleProc) {
return;
}
- if (fOwningTexture && !fOwningTexture->resourcePriv().isPurgeable()) {
+ if (fOwningTexture && fOwningTexture->resourcePriv().hasRefOrPendingIO()) {
return;
}
fIdleProc(fIdleProcContext);
diff --git a/src/gpu/vk/GrVkImage.h b/src/gpu/vk/GrVkImage.h
index 0ce223c..dab3a75 100644
--- a/src/gpu/vk/GrVkImage.h
+++ b/src/gpu/vk/GrVkImage.h
@@ -139,6 +139,7 @@
protected:
void releaseImage(GrVkGpu* gpu);
void abandonImage();
+ bool hasResource() const { return fResource; }
GrVkImageInfo fInfo;
uint32_t fInitialQueueFamily;
diff --git a/src/gpu/vk/GrVkTexture.cpp b/src/gpu/vk/GrVkTexture.cpp
index bdd8db4..6e8fe67 100644
--- a/src/gpu/vk/GrVkTexture.cpp
+++ b/src/gpu/vk/GrVkTexture.cpp
@@ -120,10 +120,13 @@
}
void GrVkTexture::onRelease() {
- // When there is an idle proc, the Resource will call the proc in releaseImage() so
- // we clear it here.
- fIdleProc = nullptr;
- fIdleProcContext = nullptr;
+ // We're about to be severed from our GrVkResource. If there is an idle proc we have to decide
+ // who will handle it. If the resource is still tied to a command buffer we let it handle it.
+ // Otherwise, we handle it.
+ if (this->hasResource() && this->resource()->isOwnedByCommandBuffer()) {
+ fIdleProc = nullptr;
+ fIdleProcContext = nullptr;
+ }
// we create this and don't hand it off, so we should always destroy it
if (fTextureView) {
@@ -137,10 +140,14 @@
}
void GrVkTexture::onAbandon() {
- // When there is an idle proc, the Resource will call the proc in abandonImage() so
- // we clear it here.
- fIdleProc = nullptr;
- fIdleProcContext = nullptr;
+ // We're about to be severed from our GrVkResource. If there is an idle proc we have to decide
+ // who will handle it. If the resource is still tied to a command buffer we let it handle it.
+ // Otherwise, we handle it.
+ if (this->hasResource() && this->resource()->isOwnedByCommandBuffer()) {
+ fIdleProc = nullptr;
+ fIdleProcContext = nullptr;
+ }
+
// we create this and don't hand it off, so we should always destroy it
if (fTextureView) {
fTextureView->unrefAndAbandon();
@@ -172,19 +179,20 @@
}
}
-void GrVkTexture::becamePurgeable() {
+void GrVkTexture::removedLastRefOrPendingIO() {
if (!fIdleProc) {
return;
}
// This is called when the GrTexture is purgeable. However, we need to check whether the
// Resource is still owned by any command buffers. If it is then it will call the proc.
- auto* resource = this->resource();
- SkASSERT(resource);
- if (resource->isOwnedByCommandBuffer()) {
+ auto* resource = this->hasResource() ? this->resource() : nullptr;
+ if (resource && resource->isOwnedByCommandBuffer()) {
return;
}
fIdleProc(fIdleProcContext);
fIdleProc = nullptr;
fIdleProcContext = nullptr;
- resource->setIdleProc(nullptr, nullptr, nullptr);
+ if (resource) {
+ resource->setIdleProc(nullptr, nullptr, nullptr);
+ }
}
diff --git a/src/gpu/vk/GrVkTexture.h b/src/gpu/vk/GrVkTexture.h
index 9005b42..97d7f06 100644
--- a/src/gpu/vk/GrVkTexture.h
+++ b/src/gpu/vk/GrVkTexture.h
@@ -69,7 +69,7 @@
const GrVkImageView*, GrMipMapsStatus, GrBackendObjectOwnership, GrWrapCacheable,
GrIOType);
- void becamePurgeable() override;
+ void removedLastRefOrPendingIO() override;
const GrVkImageView* fTextureView;
GrTexture::IdleProc* fIdleProc = nullptr;
diff --git a/src/gpu/vk/GrVkTextureRenderTarget.h b/src/gpu/vk/GrVkTextureRenderTarget.h
index 1538287..ecd6343 100644
--- a/src/gpu/vk/GrVkTextureRenderTarget.h
+++ b/src/gpu/vk/GrVkTextureRenderTarget.h
@@ -42,13 +42,15 @@
protected:
void onAbandon() override {
- GrVkRenderTarget::onAbandon();
+ // In order to correctly handle calling texture idle procs, GrVkTexture must go first.
GrVkTexture::onAbandon();
+ GrVkRenderTarget::onAbandon();
}
void onRelease() override {
- GrVkRenderTarget::onRelease();
+ // In order to correctly handle calling texture idle procs, GrVkTexture must go first.
GrVkTexture::onRelease();
+ GrVkRenderTarget::onRelease();
}
private: