commit 43f443f086b71f506f315aa100b10a88e5e6ddc7
author Brian Osman <brianosman@google.com> Fri Jun 05 11:11:36 2020 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Fri Jun 05 15:54:19 2020 +0000
tree 7fb3471a7e0d18370ecb1172896e0b83d72bd673
parent 288ecf60bde8347e2659e73f4eec354fd8d3eaf6

Fix caching of GL program binaries

Our "header" reading and writing code didn't agree, so we always failed
to recognize cached program binaries. The asserts in the testing sink
failed to notice, because we did get a 100% cache hit rate, but we
immediately discarded the data we received.

We now also check that we didn't insert anything into the cache, as a
proxy for doing any shader compile work. That change, plus the tweak to
set cached=false when the header fields are invalid (like we do if we
encounter problems further in the blob) detected the problem. Adding the
version tag to the start of the encoded blob fixes the test, and means
that program binary caching is actually working again.

This code still looks (and is) fragile. The next CL is going to rewrite
things to use SkReadBuffer and SkWriteBuffer, make the parsing code less
brittle, and give us a more robust way to detect failure anywhere in the
stream.

Bug: skia:9402
Change-Id: I0329f088e0afce3998494d91ef2206e5eb9cac42
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/294599
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

dm/DMSrcSink.cpp

diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index 3630a5c..d1ac17e 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -1563,12 +1563,13 @@
     SkBitmap reference;
     SkString refLog;
     SkDynamicMemoryWStream refStream;
-    memoryCache.resetNumCacheMisses();
+    memoryCache.resetCacheStats();
     Result refResult = this->onDraw(src, &reference, &refStream, &refLog, contextOptions);
     if (!refResult.isOk()) {
         return refResult;
     }
     SkASSERT(!memoryCache.numCacheMisses());
+    SkASSERT(!memoryCache.numCacheStores());
 
     return compare_bitmaps(reference, *dst);
 }

src/gpu/gl/builders/GrGLProgramBuilder.cpp

diff --git a/src/gpu/gl/builders/GrGLProgramBuilder.cpp b/src/gpu/gl/builders/GrGLProgramBuilder.cpp
index 54535b7..cb96a7a 100644
--- a/src/gpu/gl/builders/GrGLProgramBuilder.cpp
+++ b/src/gpu/gl/builders/GrGLProgramBuilder.cpp
@@ -167,6 +167,7 @@
         GL_CALL(GetProgramiv(programID, GL_PROGRAM_BINARY_LENGTH, &length));
         if (length > 0) {
             SkWriter32 writer;
+            writer.write32(GrPersistentCacheUtils::kCurrentVersion);
             writer.write32(kGLPB_Tag);
 
             writer.writePad(&inputs, sizeof(inputs));
@@ -300,6 +301,11 @@
                     sksl[i] = &cached_sksl[i];
                 }
                 break;
+
+            default:
+                // We got something invalid, so pretend it wasn't there
+                cached = false;
+                break;
         }
     }
     if (!usedProgramBinaries) {

tools/gpu/MemoryCache.cpp

diff --git a/tools/gpu/MemoryCache.cpp b/tools/gpu/MemoryCache.cpp
index d8fdaf7..a44034c 100644
--- a/tools/gpu/MemoryCache.cpp
+++ b/tools/gpu/MemoryCache.cpp
@@ -57,6 +57,7 @@
         SkDebugf("Store Key: %s\n\tData: %s\n\n", data_to_str(key).c_str(),
                  data_to_str(data).c_str());
     }
+    ++fCacheStoreCnt;
     fMap[Key(key)] = Value(data);
 }
 

tools/gpu/MemoryCache.h

diff --git a/tools/gpu/MemoryCache.h b/tools/gpu/MemoryCache.h
index 22911a5..f70cf13 100644
--- a/tools/gpu/MemoryCache.h
+++ b/tools/gpu/MemoryCache.h
@@ -28,14 +28,18 @@
     MemoryCache(const MemoryCache&) = delete;
     MemoryCache& operator=(const MemoryCache&) = delete;
     void reset() {
-        fCacheMissCnt = 0;
+        this->resetCacheStats();
         fMap.clear();
     }
 
     sk_sp<SkData> load(const SkData& key) override;
     void store(const SkData& key, const SkData& data) override;
     int numCacheMisses() const { return fCacheMissCnt; }
-    void resetNumCacheMisses() { fCacheMissCnt = 0; }
+    int numCacheStores() const { return fCacheStoreCnt; }
+    void resetCacheStats() {
+        fCacheMissCnt = 0;
+        fCacheStoreCnt = 0;
+    }
 
     void writeShadersToDisk(const char* path, GrBackendApi backend);
 
@@ -80,6 +84,7 @@
     };
 
     int fCacheMissCnt = 0;
+    int fCacheStoreCnt = 0;
     std::unordered_map<Key, Value, Hash> fMap;
 };