Reland "Only store resources in the GrResourceCache::fScratchMap that are available to be scratch."
This reverts commit 4a0bc2b344d7b8c178adf8bd821add2a1645c643.
Reason for revert: reland with crash workaround
Original change's description:
> Revert "Only store resources in the GrResourceCache::fScratchMap that are available to be scratch."
>
> This reverts commit 1a2326363a724ff9512b04455f7004ef810bc02f.
>
> Reason for revert: breaking win10 quadro400 perf bot on vk and vkmsaa
>
> Original change's description:
> > Only store resources in the GrResourceCache::fScratchMap that are available to be scratch.
> >
> > Currently when we create a scratch resource, we immediately add it to
> > scratch map and it will stay there until we delete the resource. The one
> > exception to this is adding a unique key will remove a resource from
> > the scratch map. This means there are resources in the scratch map that
> > can't be returned when looking for a scratch because they are either
> > already in use by something else or their budget was changed to
> > unbudgeted. This means everything time we do a scratch lookup, even
> > after finding the list of resources that match a key, we still have to
> > iterate that list to see if we can use that resource or not.
> >
> > The problem comes when we may have lots of resources that all match the
> > same key (think 1000s of identical buffers). Then the cost of iterating
> > this list starts to get very high.
> >
> > This change makes it so only resources that can actively be used as a
> > scratch at that moment are stored in the scratch map. Thus when we find
> > a scratch resource we pull it out of the scratch map. When that resources
> > refs go back to zero it is added back to the scratch map. Similar removal
> > is also now used for changing a resource to and from budgeted.
> >
> > Change-Id: I52b415d0e035dfc589f3d712be85799a56827bf0
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/367976
> > Reviewed-by: Brian Salomon <bsalomon@google.com>
> > Commit-Queue: Greg Daniel <egdaniel@google.com>
>
> TBR=egdaniel@google.com,bsalomon@google.com
>
> Change-Id: I1e57e10e75f930adfecb0e4167c1d6269798c893
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368236
> Reviewed-by: Greg Daniel <egdaniel@google.com>
> Commit-Queue: Greg Daniel <egdaniel@google.com>
TBR=egdaniel@google.com,bsalomon@google.com
# Not skipping CQ checks because this is a reland.
Change-Id: Ied3995b963f8383954fc4a53a1de9e17234e5e6c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368239
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
diff --git a/src/gpu/GrGpuResource.cpp b/src/gpu/GrGpuResource.cpp
index 19d1417..9267608 100644
--- a/src/gpu/GrGpuResource.cpp
+++ b/src/gpu/GrGpuResource.cpp
@@ -163,7 +163,7 @@
mutableThis->willRemoveLastRef();
}
-void GrGpuResource::notifyRefCntIsZero() const {
+void GrGpuResource::notifyARefCntIsZero(LastRemovedRef removedRef) const {
if (this->wasDestroyed()) {
// We've already been removed from the cache. Goodbye cruel world!
delete this;
@@ -172,7 +172,7 @@
GrGpuResource* mutableThis = const_cast<GrGpuResource*>(this);
- get_resource_cache(fGpu)->resourceAccess().notifyRefCntReachedZero(mutableThis);
+ get_resource_cache(fGpu)->resourceAccess().notifyARefCntReachedZero(mutableThis, removedRef);
}
void GrGpuResource::removeScratchKey() {
diff --git a/src/gpu/GrGpuResource.h b/src/gpu/GrGpuResource.h
index 79876d3..ee61bbe 100644
--- a/src/gpu/GrGpuResource.h
+++ b/src/gpu/GrGpuResource.h
@@ -21,9 +21,9 @@
* Separated out as a base class to isolate the ref-cnting behavior and provide friendship without
* exposing all of GrGpuResource.
*
- * PRIOR to the last ref being removed DERIVED::notifyRefCntWillBeZero() will be called
+ * PRIOR to the last ref being removed DERIVED::notifyARefCntWillBeZero() will be called
* (static poly morphism using CRTP). It is legal for additional ref's to be added
- * during this time. AFTER the ref count reaches zero DERIVED::notifyRefCntIsZero() will be
+ * during this time. AFTER the ref count reaches zero DERIVED::notifyARefCntIsZero() will be
* called.
*/
template <typename DERIVED> class GrIORef : public SkNoncopyable {
@@ -37,11 +37,16 @@
(void)fRefCnt.fetch_add(+1, std::memory_order_relaxed);
}
+ // This enum is used to notify the GrResourceCache which type of ref just dropped to zero.
+ enum class LastRemovedRef {
+ kMainRef, // This refers to fRefCnt
+ kCommandBufferUsage, // This refers to fCommandBufferUsageCnt
+ };
+
void unref() const {
SkASSERT(this->getRefCnt() > 0);
- if (1 == fRefCnt.fetch_add(-1, std::memory_order_acq_rel) &&
- this->hasNoCommandBufferUsages()) {
- this->notifyWillBeZero();
+ if (1 == fRefCnt.fetch_add(-1, std::memory_order_acq_rel)) {
+ this->notifyWillBeZero(LastRemovedRef::kMainRef);
}
}
@@ -52,9 +57,8 @@
void removeCommandBufferUsage() const {
SkASSERT(!this->hasNoCommandBufferUsages());
- if (1 == fCommandBufferUsageCnt.fetch_add(-1, std::memory_order_acq_rel) &&
- 0 == this->getRefCnt()) {
- this->notifyWillBeZero();
+ if (1 == fCommandBufferUsageCnt.fetch_add(-1, std::memory_order_acq_rel)) {
+ this->notifyWillBeZero(LastRemovedRef::kCommandBufferUsage);
}
}
@@ -63,8 +67,6 @@
#endif
protected:
- friend class GrResourceCache; // for internalHasRef
-
GrIORef() : fRefCnt(1), fCommandBufferUsageCnt(0) {}
bool internalHasRef() const { return SkToBool(this->getRefCnt()); }
@@ -80,18 +82,22 @@
}
private:
- void notifyWillBeZero() const {
- // At this point we better be the only thread accessing this resource.
- // Trick out the notifyRefCntWillBeZero() call by adding back one more ref.
- fRefCnt.fetch_add(+1, std::memory_order_relaxed);
- static_cast<const DERIVED*>(this)->notifyRefCntWillBeZero();
- // notifyRefCntWillBeZero() could have done anything, including re-refing this and
- // passing on to another thread. Take away the ref-count we re-added above and see
- // if we're back to zero.
- // TODO: Consider making it so that refs can't be added and merge
- // notifyRefCntWillBeZero()/willRemoveLastRef() with notifyRefCntIsZero().
- if (1 == fRefCnt.fetch_add(-1, std::memory_order_acq_rel)) {
- static_cast<const DERIVED*>(this)->notifyRefCntIsZero();
+ void notifyWillBeZero(LastRemovedRef removedRef) const {
+ if (0 == this->getRefCnt() && this->hasNoCommandBufferUsages()) {
+ // At this point we better be the only thread accessing this resource.
+ // Trick out the notifyRefCntWillBeZero() call by adding back one more ref.
+ fRefCnt.fetch_add(+1, std::memory_order_relaxed);
+ static_cast<const DERIVED*>(this)->notifyRefCntWillBeZero();
+ // notifyRefCntWillBeZero() could have done anything, including re-refing this and
+ // passing on to another thread. Take away the ref-count we re-added above and see
+ // if we're back to zero.
+ // TODO: Consider making it so that refs can't be added and merge
+ // notifyRefCntWillBeZero()/willRemoveLastRef() with notifyARefCntIsZero().
+ if (1 == fRefCnt.fetch_add(-1, std::memory_order_acq_rel)) {
+ static_cast<const DERIVED*>(this)->notifyARefCntIsZero(removedRef);
+ }
+ } else {
+ static_cast<const DERIVED*>(this)->notifyARefCntIsZero(removedRef);
}
}
@@ -297,7 +303,7 @@
void setUniqueKey(const GrUniqueKey&);
void removeUniqueKey();
void notifyRefCntWillBeZero() const;
- void notifyRefCntIsZero() const;
+ void notifyARefCntIsZero(LastRemovedRef removedRef) const;
void removeScratchKey();
void makeBudgeted();
void makeUnbudgeted();
@@ -328,7 +334,8 @@
const UniqueID fUniqueID;
using INHERITED = GrIORef<GrGpuResource>;
- friend class GrIORef<GrGpuResource>; // to access notifyRefCntWillBeZero and notifyRefCntIsZero.
+ friend class GrIORef<GrGpuResource>; // to access notifyRefCntWillBeZero and
+ // notifyARefCntIsZero.
};
class GrGpuResource::ProxyAccess {
diff --git a/src/gpu/GrGpuResourceCacheAccess.h b/src/gpu/GrGpuResourceCacheAccess.h
index e71f1a1..43eded6 100644
--- a/src/gpu/GrGpuResourceCacheAccess.h
+++ b/src/gpu/GrGpuResourceCacheAccess.h
@@ -32,6 +32,10 @@
GrBudgetedType::kBudgeted == fResource->resourcePriv().budgetedType();
}
+ bool isUsableAsScratch() const {
+ return this->isScratch() && !fResource->internalHasRef();
+ }
+
/**
* Called by the cache to delete the resource under normal circumstances.
*/
diff --git a/src/gpu/GrResourceCache.cpp b/src/gpu/GrResourceCache.cpp
index c1748a3..82ec5b5 100644
--- a/src/gpu/GrResourceCache.cpp
+++ b/src/gpu/GrResourceCache.cpp
@@ -155,12 +155,7 @@
fBudgetedHighWaterBytes = std::max(fBudgetedBytes, fBudgetedHighWaterBytes);
#endif
}
- if (resource->resourcePriv().getScratchKey().isValid() &&
- !resource->getUniqueKey().isValid()) {
- SkASSERT(!resource->resourcePriv().refsWrappedObjects());
- fScratchMap.insert(resource->resourcePriv().getScratchKey(), resource);
- }
-
+ SkASSERT(!resource->cacheAccess().isUsableAsScratch());
this->purgeAsNeeded();
}
@@ -186,8 +181,7 @@
fBudgetedBytes, "free", fMaxBytes - fBudgetedBytes);
}
- if (resource->resourcePriv().getScratchKey().isValid() &&
- !resource->getUniqueKey().isValid()) {
+ if (resource->cacheAccess().isUsableAsScratch()) {
fScratchMap.remove(resource->resourcePriv().getScratchKey(), resource);
}
if (resource->getUniqueKey().isValid()) {
@@ -285,13 +279,8 @@
AvailableForScratchUse() { }
bool operator()(const GrGpuResource* resource) const {
- SkASSERT(!resource->getUniqueKey().isValid() &&
- resource->resourcePriv().getScratchKey().isValid());
-
- // isScratch() also tests that the resource is budgeted.
- if (resource->internalHasRef() || !resource->cacheAccess().isScratch()) {
- return false;
- }
+ // Everything that is in the scratch map should be usable as a
+ // scratch resource.
return true;
}
};
@@ -301,6 +290,7 @@
GrGpuResource* resource = fScratchMap.find(scratchKey, AvailableForScratchUse());
if (resource) {
+ fScratchMap.remove(scratchKey, resource);
this->refAndMakeResourceMRU(resource);
this->validate();
}
@@ -310,7 +300,7 @@
void GrResourceCache::willRemoveScratchKey(const GrGpuResource* resource) {
ASSERT_SINGLE_OWNER
SkASSERT(resource->resourcePriv().getScratchKey().isValid());
- if (!resource->getUniqueKey().isValid()) {
+ if (resource->cacheAccess().isUsableAsScratch()) {
fScratchMap.remove(resource->resourcePriv().getScratchKey(), resource);
}
}
@@ -324,7 +314,7 @@
fUniqueHash.remove(resource->getUniqueKey());
}
resource->cacheAccess().removeUniqueKey();
- if (resource->resourcePriv().getScratchKey().isValid()) {
+ if (resource->cacheAccess().isUsableAsScratch()) {
fScratchMap.insert(resource->resourcePriv().getScratchKey(), resource);
}
@@ -361,8 +351,9 @@
SkASSERT(nullptr == fUniqueHash.find(resource->getUniqueKey()));
} else {
// 'resource' didn't have a valid unique key before so it is switching sides. Remove it
- // from the ScratchMap
- if (resource->resourcePriv().getScratchKey().isValid()) {
+ // from the ScratchMap. The isUsableAsScratch call depends on us not adding the new
+ // unique key until after this check.
+ if (resource->cacheAccess().isUsableAsScratch()) {
fScratchMap.remove(resource->resourcePriv().getScratchKey(), resource);
}
}
@@ -397,7 +388,8 @@
this->validate();
}
-void GrResourceCache::notifyRefCntReachedZero(GrGpuResource* resource) {
+void GrResourceCache::notifyARefCntReachedZero(GrGpuResource* resource,
+ GrGpuResource::LastRemovedRef removedRef) {
ASSERT_SINGLE_OWNER
SkASSERT(resource);
SkASSERT(!resource->wasDestroyed());
@@ -406,6 +398,17 @@
// will be moved to the queue if it is newly purgeable.
SkASSERT(fNonpurgeableResources[*resource->cacheAccess().accessCacheIndex()] == resource);
+ if (removedRef == GrGpuResource::LastRemovedRef::kMainRef) {
+ if (resource->cacheAccess().isUsableAsScratch()) {
+ fScratchMap.insert(resource->resourcePriv().getScratchKey(), resource);
+ }
+ }
+
+ if (resource->cacheAccess().hasRefOrCommandBufferUsage()) {
+ this->validate();
+ return;
+ }
+
#ifdef SK_DEBUG
// When the timestamp overflows validate() is called. validate() checks that resources in
// the nonpurgeable array are indeed not purgeable. However, the movement from the array to
@@ -489,6 +492,9 @@
!resource->cacheAccess().hasRefOrCommandBufferUsage()) {
++fNumBudgetedResourcesFlushWillMakePurgeable;
}
+ if (resource->cacheAccess().isUsableAsScratch()) {
+ fScratchMap.insert(resource->resourcePriv().getScratchKey(), resource);
+ }
this->purgeAsNeeded();
} else {
SkASSERT(resource->resourcePriv().budgetedType() != GrBudgetedType::kUnbudgetedCacheable);
@@ -498,6 +504,10 @@
!resource->cacheAccess().hasRefOrCommandBufferUsage()) {
--fNumBudgetedResourcesFlushWillMakePurgeable;
}
+ if (!resource->cacheAccess().hasRef() && !resource->getUniqueKey().isValid() &&
+ resource->resourcePriv().getScratchKey().isValid()) {
+ fScratchMap.remove(resource->resourcePriv().getScratchKey(), resource);
+ }
}
SkASSERT(wasPurgeable == resource->resourcePriv().isPurgeable());
TRACE_COUNTER2("skia.gpu.cache", "skia budget", "used",
@@ -857,29 +867,24 @@
const GrScratchKey& scratchKey = resource->resourcePriv().getScratchKey();
const GrUniqueKey& uniqueKey = resource->getUniqueKey();
- if (resource->cacheAccess().isScratch()) {
+ if (resource->cacheAccess().isUsableAsScratch()) {
SkASSERT(!uniqueKey.isValid());
+ SkASSERT(GrBudgetedType::kBudgeted == resource->resourcePriv().budgetedType());
+ SkASSERT(!resource->cacheAccess().hasRef());
++fScratch;
SkASSERT(fScratchMap->countForKey(scratchKey));
SkASSERT(!resource->resourcePriv().refsWrappedObjects());
} else if (scratchKey.isValid()) {
SkASSERT(GrBudgetedType::kBudgeted != resource->resourcePriv().budgetedType() ||
- uniqueKey.isValid());
- if (!uniqueKey.isValid()) {
- ++fCouldBeScratch;
- SkASSERT(fScratchMap->countForKey(scratchKey));
- }
+ uniqueKey.isValid() || resource->cacheAccess().hasRef());
SkASSERT(!resource->resourcePriv().refsWrappedObjects());
+ SkASSERT(!fScratchMap->has(resource, scratchKey));
}
if (uniqueKey.isValid()) {
++fContent;
SkASSERT(fUniqueHash->find(uniqueKey) == resource);
SkASSERT(GrBudgetedType::kBudgeted == resource->resourcePriv().budgetedType() ||
resource->resourcePriv().refsWrappedObjects());
-
- if (scratchKey.isValid()) {
- SkASSERT(!fScratchMap->has(resource, scratchKey));
- }
}
if (GrBudgetedType::kBudgeted == resource->resourcePriv().budgetedType()) {
@@ -892,8 +897,7 @@
{
int count = 0;
fScratchMap.foreach([&](const GrGpuResource& resource) {
- SkASSERT(resource.resourcePriv().getScratchKey().isValid());
- SkASSERT(!resource.getUniqueKey().isValid());
+ SkASSERT(resource.cacheAccess().isUsableAsScratch());
count++;
});
SkASSERT(count == fScratchMap.count());
@@ -941,7 +945,7 @@
SkASSERT(fBudgetedCount <= fBudgetedHighWaterCount);
#endif
SkASSERT(stats.fContent == fUniqueHash.count());
- SkASSERT(stats.fScratch + stats.fCouldBeScratch == fScratchMap.count());
+ SkASSERT(stats.fScratch == fScratchMap.count());
// This assertion is not currently valid because we can be in recursive notifyCntReachedZero()
// calls. This will be fixed when subresource registration is explicit.
diff --git a/src/gpu/GrResourceCache.h b/src/gpu/GrResourceCache.h
index a9b893b..2c13917 100644
--- a/src/gpu/GrResourceCache.h
+++ b/src/gpu/GrResourceCache.h
@@ -249,7 +249,7 @@
////
void insertResource(GrGpuResource*);
void removeResource(GrGpuResource*);
- void notifyRefCntReachedZero(GrGpuResource*);
+ void notifyARefCntReachedZero(GrGpuResource*, GrGpuResource::LastRemovedRef);
void changeUniqueKey(GrGpuResource*, const GrUniqueKey&);
void removeUniqueKey(GrGpuResource*);
void willRemoveScratchKey(const GrGpuResource*);
@@ -406,10 +406,12 @@
kRefCntReachedZero_RefNotificationFlag = 0x2,
};
/**
- * Called by GrGpuResources when they detect that their ref cnt has reached zero.
+ * Called by GrGpuResources when they detect one of their ref cnts have reached zero. This may
+ * either be the main ref or the command buffer usage ref.
*/
- void notifyRefCntReachedZero(GrGpuResource* resource) {
- fCache->notifyRefCntReachedZero(resource);
+ void notifyARefCntReachedZero(GrGpuResource* resource,
+ GrGpuResource::LastRemovedRef removedRef) {
+ fCache->notifyARefCntReachedZero(resource, removedRef);
}
/**
diff --git a/tests/ResourceCacheTest.cpp b/tests/ResourceCacheTest.cpp
index 23d46f3..a6ecd40 100644
--- a/tests/ResourceCacheTest.cpp
+++ b/tests/ResourceCacheTest.cpp
@@ -817,7 +817,8 @@
// Scratch resources are registered with GrResourceCache just by existing. There are 2.
REPORTER_ASSERT(reporter, 2 == TestResource::NumAlive());
- SkDEBUGCODE(REPORTER_ASSERT(reporter, 2 == cache->countScratchEntriesForKey(scratchKey));)
+ // As long as there are outstanding refs on the resources they will not be in the scratch map
+ SkDEBUGCODE(REPORTER_ASSERT(reporter, 0 == cache->countScratchEntriesForKey(scratchKey));)
REPORTER_ASSERT(reporter, 2 == cache->getResourceCount());
REPORTER_ASSERT(reporter, a->gpuMemorySize() + b->gpuMemorySize() ==
cache->getResourceBytes());
@@ -831,6 +832,7 @@
a->unref();
b->unref();
REPORTER_ASSERT(reporter, 2 == TestResource::NumAlive());
+ // Since we removed the refs to the resources they will now be in the scratch map
SkDEBUGCODE(REPORTER_ASSERT(reporter, 2 == cache->countScratchEntriesForKey(scratchKey));)
// Purge again. This time resources should be purgeable.
