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/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.