[graphite] Add validation checks to ResourceCache

Bug: skia:12754
Change-Id: Ie88290f18864d61138964eb618dc6ac2d6ff4fe0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/508916
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>

experimental/graphite/src/Buffer.h

@@ -24,7 +24,10 @@ public:
 
 protected:
     Buffer(const Gpu* gpu, size_t size, BufferType type, PrioritizeGpuReads prioritizeGpuReads)
-        : Resource(gpu), fSize(size), fType(type), fPrioritizeGpuReads(prioritizeGpuReads) {}
+        : Resource(gpu, Ownership::kOwned)
+        , fSize(size)
+        , fType(type)
+        , fPrioritizeGpuReads(prioritizeGpuReads) {}
 
     void* fMapPtr = nullptr;
 

experimental/graphite/src/GraphicsPipeline.cpp

@@ -9,7 +9,7 @@
 
 namespace skgpu {
 
-GraphicsPipeline::GraphicsPipeline(const Gpu* gpu) : Resource(gpu) {
+GraphicsPipeline::GraphicsPipeline(const Gpu* gpu) : Resource(gpu, Ownership::kOwned) {
 }
 
 GraphicsPipeline::~GraphicsPipeline() {

experimental/graphite/src/Resource.cpp

@@ -11,11 +11,12 @@
 
 namespace skgpu {
 
-Resource::Resource(const Gpu* gpu)
+Resource::Resource(const Gpu* gpu, Ownership ownership)
         : fGpu(gpu)
         , fUsageRefCnt(1)
         , fCommandBufferRefCnt(0)
-        , fCacheRefCnt(0) {}
+        , fCacheRefCnt(0)
+        , fOwnership(ownership) {}
 
 Resource::~Resource() {
     // The cache should have released or destroyed this resource.

experimental/graphite/src/Resource.h

@@ -79,6 +79,8 @@ public:
         }
     }
 
+    Ownership ownership() const { return fOwnership; }
+
     // Tests whether a object has been abandoned or released. All objects will be in this state
     // after their creating Context is destroyed or abandoned.
     //
@@ -95,7 +97,7 @@ public:
     void setKey(const GraphiteResourceKey& key) { fKey = key; }
 
 protected:
-    Resource(const Gpu*);
+    Resource(const Gpu*, Ownership);
     virtual ~Resource();
 
     // Overridden to free GPU resources in the backend API.
@@ -152,10 +154,15 @@ private:
         }
     }
 
+#ifdef SK_DEBUG
+    bool isUsableAsScratch() const {
+        return fKey.shareable() == Shareable::kNo && !this->hasUsageRef() && fNonShareableInCache;
+    }
+#endif
+
     ////////////////////////////////////////////////////////////////////////////
     // The remaining calls are meant to be truely private
     ////////////////////////////////////////////////////////////////////////////
-
     bool hasUsageRef() const {
         if (0 == fUsageRefCnt.load(std::memory_order_acquire)) {
             // The acquire barrier is only really needed if we return true.  It
@@ -217,12 +224,20 @@ private:
     // be returned or not.
     mutable int fReturnIndex = -1;
 
+    Ownership fOwnership;
+
     // An index into a heap when this resource is purgeable or an array when not. This is maintained
     // by the cache.
     mutable int fCacheArrayIndex = -1;
     // This value reflects how recently this resource was accessed in the cache. This is maintained
     // by the cache.
     uint32_t fTimestamp;
+
+    // This is only used during validation checking. Lots of the validation code depends on a
+    // resource being purgeable or not. However, purgeable itself just means having no refs. The
+    // refs can be removed before a Resource is returned to the cache (or even added to the
+    // ReturnQueue).
+    SkDEBUGCODE(mutable bool fNonShareableInCache = false);
 };
 
 } // namespace skgpu

experimental/graphite/src/ResourceCache.cpp

@@ -10,6 +10,7 @@
 #include "experimental/graphite/src/GraphiteResourceKey.h"
 #include "experimental/graphite/src/Resource.h"
 #include "include/private/SingleOwner.h"
+#include "include/utils/SkRandom.h"
 #include "src/core/SkTMultiMap.h"
 
 namespace skgpu {
@@ -78,6 +79,8 @@ void ResourceCache::insertResource(Resource* resource) {
 
     this->addToNonpurgeableArray(resource);
 
+    SkDEBUGCODE(fCount++;)
+
     if (resource->key().shareable() == Shareable::kYes) {
         fResourceMap.insert(resource->key(), resource);
     }
@@ -98,6 +101,7 @@ Resource* ResourceCache::findAndRefResource(const skgpu::GraphiteResourceKey& ke
             // If a resource is not shareable (i.e. scratch resource) then we remove it from the map
             // so that it isn't found again.
             fResourceMap.remove(key, resource);
+            SkDEBUGCODE(resource->fNonShareableInCache = false;)
         }
         this->refAndMakeResourceMRU(resource);
         this->validate();
@@ -215,6 +219,7 @@ void ResourceCache::returnResourceToCache(Resource* resource, LastRemovedRef rem
             // Shareable resources should still be in the cache
             SkASSERT(fResourceMap.find(resource->key()));
         } else {
+            SkDEBUGCODE(resource->fNonShareableInCache = true;)
             fResourceMap.insert(resource->key(), resource);
         }
     }
@@ -236,8 +241,11 @@ void ResourceCache::returnResourceToCache(Resource* resource, LastRemovedRef rem
         return;
     }
 
+    resource->setTimestamp(this->getNextTimestamp());
+
     this->removeFromNonpurgeableArray(resource);
     fPurgeableQueue.insert(resource);
+    this->validate();
 }
 
 void ResourceCache::addToNonpurgeableArray(Resource* resource) {
@@ -357,7 +365,87 @@ int* ResourceCache::AccessResourceIndex(Resource* const& res) {
 
 #ifdef SK_DEBUG
 void ResourceCache::validate() const {
-    // TODO: fill this out
+    // Reduce the frequency of validations for large resource counts.
+    static SkRandom gRandom;
+    int mask = (SkNextPow2(fCount + 1) >> 5) - 1;
+    if (~mask && (gRandom.nextU() & mask)) {
+        return;
+    }
+
+    struct Stats {
+        int fShareable;
+        int fScratch;
+        const ResourceMap* fResourceMap;
+
+        Stats(const ResourceCache* cache) {
+            memset(this, 0, sizeof(*this));
+            fResourceMap = &cache->fResourceMap;
+        }
+
+        void update(Resource* resource) {
+            const GraphiteResourceKey& key = resource->key();
+            SkASSERT(key.isValid());
+
+            // We should always have at least 1 cache ref
+            SkASSERT(resource->hasCacheRef());
+
+            // We track scratch (non-shareable, no usage refs, has been returned to cache) and
+            // shareable resources here as those should be the only things in the fResourceMap. A
+            // non-shareable resources that does meet the scratch criteria will not be able to be
+            // given back out from a cache requests. After processing all the resources we assert
+            // that the fScratch + fShareable equals the count in the fResourceMap.
+            if (resource->isUsableAsScratch()) {
+                SkASSERT(key.shareable() == Shareable::kNo);
+                SkASSERT(!resource->hasUsageRef());
+                ++fScratch;
+                SkASSERT(fResourceMap->has(resource, key));
+                SkASSERT(resource->ownership() == Ownership::kOwned);
+            } else if (key.shareable() == Shareable::kNo) {
+                SkASSERT(resource->ownership() == Ownership::kOwned);
+                SkASSERT(!fResourceMap->has(resource, key));
+            } else {
+                SkASSERT(key.shareable() == Shareable::kYes);
+                ++fShareable;
+                SkASSERT(fResourceMap->has(resource, key));
+            }
+        }
+    };
+
+    {
+        int count = 0;
+        fResourceMap.foreach([&](const Resource& resource) {
+            SkASSERT(resource.isUsableAsScratch() || resource.key().shareable() == Shareable::kYes);
+            count++;
+        });
+        SkASSERT(count == fResourceMap.count());
+    }
+
+    // In the below checks we can assert that anything in the purgeable queue is purgeable because
+    // we won't put a Resource into that queue unless all refs are zero. Thus there is no way for
+    // that resource to be made non-purgeable without going through the cache (which will switch
+    // queues back to non-purgeable).
+    //
+    // However, we can't say the same for things in the non-purgeable array. It is possible that
+    // Resources have removed all their refs (thus technically become purgeable) but have not been
+    // processed back into the cache yet. Thus we may not have moved resources to the purgeable
+    // queue yet. Its also possible that Resource hasn't been added to the ReturnQueue yet (thread
+    // paused between unref and adding to ReturnQueue) so we can't even make asserts like not
+    // purgeable or is in ReturnQueue.
+    Stats stats(this);
+    for (int i = 0; i < fNonpurgeableResources.count(); ++i) {
+        SkASSERT(*fNonpurgeableResources[i]->accessCacheIndex() == i);
+        SkASSERT(!fNonpurgeableResources[i]->wasDestroyed());
+        SkASSERT(!this->inPurgeableQueue(fNonpurgeableResources[i]));
+        stats.update(fNonpurgeableResources[i]);
+    }
+    for (int i = 0; i < fPurgeableQueue.count(); ++i) {
+        SkASSERT(fPurgeableQueue.at(i)->isPurgeable());
+        SkASSERT(*fPurgeableQueue.at(i)->accessCacheIndex() == i);
+        SkASSERT(!fPurgeableQueue.at(i)->wasDestroyed());
+        stats.update(fPurgeableQueue.at(i));
+    }
+
+    SkASSERT((stats.fScratch + stats.fShareable) == fResourceMap.count());
 }
 
 bool ResourceCache::isInCache(const Resource* resource) const {

experimental/graphite/src/ResourceCache.h

@@ -98,6 +98,8 @@ private:
     PurgeableQueue fPurgeableQueue;
     ResourceArray fNonpurgeableResources;
 
+    SkDEBUGCODE(int fCount = 0;)
+
     ResourceMap fResourceMap;
 
     SingleOwner* fSingleOwner = nullptr;

experimental/graphite/src/Sampler.cpp

@@ -9,7 +9,7 @@
 
 namespace skgpu {
 
-Sampler::Sampler(const Gpu* gpu) : Resource(gpu) {}
+Sampler::Sampler(const Gpu* gpu) : Resource(gpu, Ownership::kOwned) {}
 
 Sampler::~Sampler() {}
 

experimental/graphite/src/Texture.cpp

@@ -10,7 +10,9 @@
 namespace skgpu {
 
 Texture::Texture(const Gpu* gpu, SkISize dimensions, const TextureInfo& info, Ownership ownership)
-        : Resource(gpu), fDimensions(dimensions), fInfo(info), fOwnership(ownership) {}
+        : Resource(gpu, ownership)
+        , fDimensions(dimensions)
+        , fInfo(info) {}
 
 Texture::~Texture() {}
 

experimental/graphite/src/Texture.h

@@ -28,12 +28,9 @@ public:
 protected:
     Texture(const Gpu*, SkISize dimensions, const TextureInfo& info, Ownership);
 
-    Ownership ownership() const { return fOwnership; }
-
 private:
     SkISize fDimensions;
     TextureInfo fInfo;
-    Ownership fOwnership;
 };
 
 } // namepsace skgpu

experimental/graphite/src/mtl/MtlBlitCommandEncoder.h

@@ -81,7 +81,7 @@ public:
 
 private:
     BlitCommandEncoder(const skgpu::Gpu* gpu, sk_cfp<id<MTLBlitCommandEncoder>> encoder)
-        : Resource(gpu), fCommandEncoder(std::move(encoder)) {}
+        : Resource(gpu, Ownership::kOwned), fCommandEncoder(std::move(encoder)) {}
 
     void freeGpuData() override {
         fCommandEncoder.reset();

experimental/graphite/src/mtl/MtlRenderCommandEncoder.h

@@ -246,7 +246,7 @@ private:
     inline static constexpr int kMaxExpectedTextures = 16;
 
     RenderCommandEncoder(const Gpu* gpu, sk_cfp<id<MTLRenderCommandEncoder>> encoder)
-            : Resource(gpu), fCommandEncoder(std::move(encoder)) {
+            : Resource(gpu, Ownership::kOwned), fCommandEncoder(std::move(encoder)) {
         for (int i = 0; i < kMaxExpectedBuffers; i++) {
             fCurrentVertexBuffer[i] = nil;
             fCurrentFragmentBuffer[i] = nil;