Fix the GLSL persistent cache (and add a config to test it)
We need to store (up to) three GLSL strings in the cache entry,
along with the bookkeeping to reconstruct them. To make things
simpler, we now store the null terminators.
Change-Id: Ic4fe03cb5d774464372ceec8740da1bfe9069550
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/205823
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
diff --git a/dm/DM.cpp b/dm/DM.cpp
index c1efcf7..bdf9f6e 100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -929,7 +929,8 @@
contextType, contextOverrides, gpuConfig->getSurfType(),
gpuConfig->getSamples(), gpuConfig->getUseDIText(),
gpuConfig->getColorType(), gpuConfig->getAlphaType(),
- sk_ref_sp(gpuConfig->getColorSpace()), FLAGS_gpu_threading, grCtxOptions);
+ sk_ref_sp(gpuConfig->getColorSpace()), FLAGS_gpu_threading, grCtxOptions,
+ gpuConfig->getTestPersistentCache());
} else {
return new GPUSink(contextType, contextOverrides, gpuConfig->getSurfType(),
gpuConfig->getSamples(), gpuConfig->getUseDIText(),
diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index 2c10078..86a51ef 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -1510,9 +1510,11 @@
SkAlphaType alphaType,
sk_sp<SkColorSpace> colorSpace,
bool threaded,
- const GrContextOptions& grCtxOptions)
+ const GrContextOptions& grCtxOptions,
+ int cacheType)
: INHERITED(ct, overrides, surfType, samples, diText, colorType, alphaType,
- std::move(colorSpace), threaded, grCtxOptions) {}
+ std::move(colorSpace), threaded, grCtxOptions)
+ , fCacheType(cacheType) {}
Error GPUPersistentCacheTestingSink::draw(const Src& src, SkBitmap* dst, SkWStream* wStream,
SkString* log) const {
@@ -1521,6 +1523,7 @@
sk_gpu_test::MemoryCache memoryCache;
GrContextOptions contextOptions = this->baseContextOptions();
contextOptions.fPersistentCache = &memoryCache;
+ contextOptions.fDisallowGLSLBinaryCaching = (fCacheType == 2);
Error err = this->onDraw(src, dst, wStream, log, contextOptions);
if (!err.isEmpty() || !dst) {
diff --git a/dm/DMSrcSink.h b/dm/DMSrcSink.h
index e5b2c53..54f7c4d 100644
--- a/dm/DMSrcSink.h
+++ b/dm/DMSrcSink.h
@@ -394,7 +394,8 @@
SkCommandLineConfigGpu::SurfType surfType, int samples,
bool diText, SkColorType colorType, SkAlphaType alphaType,
sk_sp<SkColorSpace> colorSpace, bool threaded,
- const GrContextOptions& grCtxOptions);
+ const GrContextOptions& grCtxOptions,
+ int cacheType);
Error draw(const Src&, SkBitmap*, SkWStream*, SkString*) const override;
@@ -404,6 +405,8 @@
}
private:
+ int fCacheType;
+
typedef GPUSink INHERITED;
};
diff --git a/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-DDL1.json b/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-DDL1.json
index d380643..0efd870 100644
--- a/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-DDL1.json
+++ b/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-DDL1.json
@@ -287,6 +287,18 @@
"gm",
"_",
"glyph_pos_h_b",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "atlastext",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "dftext",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "glyph_pos_h_b",
"_",
"svg",
"_",
diff --git a/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-DDL3.json b/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-DDL3.json
index 94005f6..b381bf1 100644
--- a/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-DDL3.json
+++ b/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-DDL3.json
@@ -288,6 +288,18 @@
"gm",
"_",
"glyph_pos_h_b",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "atlastext",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "dftext",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "glyph_pos_h_b",
"_",
"svg",
"_",
diff --git a/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-Lottie.json b/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-Lottie.json
index 7ad4453..0526c13 100644
--- a/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-Lottie.json
+++ b/infra/bots/recipes/test.expected/Test-Ubuntu17-Clang-Golo-GPU-QuadroP400-x86_64-Debug-All-Lottie.json
@@ -230,6 +230,18 @@
"gm",
"_",
"glyph_pos_h_b",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "atlastext",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "dftext",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "glyph_pos_h_b",
"_",
"svg",
"_",
diff --git a/infra/bots/recipes/test.expected/Test-Ubuntu17-GCC-Golo-GPU-QuadroP400-x86_64-Release-All-Valgrind_AbandonGpuContext_SK_CPU_LIMIT_SSE41.json b/infra/bots/recipes/test.expected/Test-Ubuntu17-GCC-Golo-GPU-QuadroP400-x86_64-Release-All-Valgrind_AbandonGpuContext_SK_CPU_LIMIT_SSE41.json
index eed0889..da0cbd5 100644
--- a/infra/bots/recipes/test.expected/Test-Ubuntu17-GCC-Golo-GPU-QuadroP400-x86_64-Release-All-Valgrind_AbandonGpuContext_SK_CPU_LIMIT_SSE41.json
+++ b/infra/bots/recipes/test.expected/Test-Ubuntu17-GCC-Golo-GPU-QuadroP400-x86_64-Release-All-Valgrind_AbandonGpuContext_SK_CPU_LIMIT_SSE41.json
@@ -190,6 +190,7 @@
"glmsaa8",
"gl1010102",
"gltestpersistentcache",
+ "gltestglslcache",
"--src",
"tests",
"gm",
@@ -213,6 +214,18 @@
"gm",
"_",
"glyph_pos_h_b",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "atlastext",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "dftext",
+ "gltestglslcache",
+ "gm",
+ "_",
+ "glyph_pos_h_b",
"_",
"svg",
"_",
diff --git a/infra/bots/recipes/test.py b/infra/bots/recipes/test.py
index 7d54051..3436dbc 100644
--- a/infra/bots/recipes/test.py
+++ b/infra/bots/recipes/test.py
@@ -226,13 +226,16 @@
# Decoding transparent images to 1010102 just looks bad
blacklist('vk1010102 image _ _')
else:
- configs.extend(['gl1010102', 'gltestpersistentcache'])
+ configs.extend(['gl1010102', 'gltestpersistentcache', 'gltestglslcache'])
# Decoding transparent images to 1010102 just looks bad
blacklist('gl1010102 image _ _')
# These tests produce slightly different pixels run to run on NV.
blacklist('gltestpersistentcache gm _ atlastext')
blacklist('gltestpersistentcache gm _ dftext')
blacklist('gltestpersistentcache gm _ glyph_pos_h_b')
+ blacklist('gltestglslcache gm _ atlastext')
+ blacklist('gltestglslcache gm _ dftext')
+ blacklist('gltestglslcache gm _ glyph_pos_h_b')
# Test rendering to wrapped dsts on a few bots
# Also test 'glenarrow', which hits F16 surfaces and F16 vertex colors.
diff --git a/src/gpu/gl/builders/GrGLProgramBuilder.cpp b/src/gpu/gl/builders/GrGLProgramBuilder.cpp
index 96a7504..a2b0621 100644
--- a/src/gpu/gl/builders/GrGLProgramBuilder.cpp
+++ b/src/gpu/gl/builders/GrGLProgramBuilder.cpp
@@ -148,8 +148,50 @@
}
}
+struct GrGLSLSet {
+ SkSL::String fGLSL[kGrShaderTypeCount];
+
+ SkSL::String& gs() { return fGLSL[kGeometry_GrShaderType]; }
+ SkSL::String& vs() { return fGLSL[kVertex_GrShaderType]; }
+ SkSL::String& fs() { return fGLSL[kFragment_GrShaderType]; }
+
+ size_t getCacheSize() const {
+ size_t size = 0;
+ for (int i = 0; i < kGrShaderTypeCount; ++i) {
+ if (fGLSL[i].size() > 0) {
+ size += fGLSL[i].size() + 1;
+ }
+ }
+ return size;
+ }
+};
+
+struct GrGLSLCacheEntry {
+ GrGLSLCacheEntry(const SkSL::Program::Inputs& inputs, const GrGLSLSet& glsl)
+ : fInputs(inputs) {
+ size_t offset = sizeof(*this);
+ for (int i = 0; i < kGrShaderTypeCount; ++i) {
+ if (glsl.fGLSL[i].size() > 0) {
+ fOffset[i] = offset;
+ offset += glsl.fGLSL[i].size() + 1;
+ } else {
+ fOffset[i] = 0;
+ }
+ }
+ }
+
+ const char* get(int shaderType) const {
+ SkASSERT(shaderType < kGrShaderTypeCount);
+ return fOffset[shaderType] ? SkTAddOffset<const char>(this, fOffset[shaderType])
+ : nullptr;
+ }
+
+ SkSL::Program::Inputs fInputs;
+ size_t fOffset[kGrShaderTypeCount];
+};
+
void GrGLProgramBuilder::storeShaderInCache(const SkSL::Program::Inputs& inputs, GrGLuint programID,
- const SkSL::String& glsl) {
+ const GrGLSLSet& glsl) {
if (!this->gpu()->getContext()->priv().getPersistentCache()) {
return;
}
@@ -175,12 +217,18 @@
}
} else {
// source cache
- size_t dataLength = sizeof(inputs) + glsl.length();
+ size_t dataLength = sizeof(GrGLSLCacheEntry) + glsl.getCacheSize();
std::unique_ptr<uint8_t[]> data(new uint8_t[dataLength]);
size_t offset = 0;
- memcpy(data.get() + offset, &inputs, sizeof(inputs));
- offset += sizeof(inputs);
- memcpy(data.get() + offset, glsl.data(), glsl.length());
+ GrGLSLCacheEntry entry(inputs, glsl);
+ memcpy(data.get() + offset, &entry, sizeof(entry));
+ offset += sizeof(entry);
+ for (int i = 0; i < kGrShaderTypeCount; ++i) {
+ if (glsl.fGLSL[i].size() > 0) {
+ memcpy(data.get() + offset, glsl.fGLSL[i].data(), glsl.fGLSL[i].size() + 1);
+ offset += glsl.fGLSL[i].size() + 1;
+ }
+ }
this->gpu()->getContext()->priv().getPersistentCache()->store(
*key, *SkData::MakeWithoutCopy(data.get(), dataLength));
}
@@ -220,13 +268,13 @@
checkLinked = true;
#endif
bool cached = fCached.get() != nullptr;
- SkSL::String glsl;
+ GrGLSLSet glsl;
if (cached) {
const uint8_t* bytes = fCached->bytes();
- size_t offset = 0;
- memcpy(&inputs, bytes + offset, sizeof(inputs));
- offset += sizeof(inputs);
if (fGpu->glCaps().programBinarySupport()) {
+ size_t offset = 0;
+ memcpy(&inputs, bytes + offset, sizeof(inputs));
+ offset += sizeof(inputs);
// binary cache hit, just hand the binary to GL
int binaryFormat;
memcpy(&binaryFormat, bytes + offset, sizeof(binaryFormat));
@@ -248,13 +296,19 @@
}
} else {
// source cache hit, we don't need to compile the SkSL->GLSL
- glsl = SkSL::String(((const char*) bytes) + offset, fCached->size() - offset);
+ const GrGLSLCacheEntry* entry = (const GrGLSLCacheEntry*)(bytes);
+ inputs = entry->fInputs;
+ for (int i = 0; i < kGrShaderTypeCount; ++i) {
+ if (entry->fOffset[i]) {
+ glsl.fGLSL[i] = SkSL::String(entry->get(i));
+ }
+ }
}
}
if (!cached || !fGpu->glCaps().programBinarySupport()) {
// either a cache miss, or we can't store binaries in the cache
- if (glsl == "" || true) {
- // don't have cached GLSL, need to compile SkSL->GLSL
+ if (glsl.fs().empty()) {
+ // Don't have cached GLSL, need to compile SkSL->GLSL
if (fFS.fForceHighPrecision) {
settings.fForceHighPrecision = true;
}
@@ -264,35 +318,42 @@
fFS.fCompilerStringLengths.begin(),
fFS.fCompilerStrings.count(),
settings,
- &glsl);
+ &glsl.fs());
if (!fs) {
this->cleanupProgram(programID, shadersToDelete);
return nullptr;
}
inputs = fs->fInputs;
+ this->addInputVars(inputs);
} else {
// we've pulled GLSL and inputs from the cache, but still need to do some setup
this->addInputVars(inputs);
this->computeCountsAndStrides(programID, primProc, false);
}
- this->addInputVars(inputs);
- if (!this->compileAndAttachShaders(glsl.c_str(), glsl.size(), programID,
+ if (!this->compileAndAttachShaders(glsl.fs().c_str(), glsl.fs().size(), programID,
GR_GL_FRAGMENT_SHADER, &shadersToDelete, settings,
inputs)) {
this->cleanupProgram(programID, shadersToDelete);
return nullptr;
}
- std::unique_ptr<SkSL::Program> vs = GrSkSLtoGLSL(gpu()->glContext(),
- GR_GL_VERTEX_SHADER,
- fVS.fCompilerStrings.begin(),
- fVS.fCompilerStringLengths.begin(),
- fVS.fCompilerStrings.count(),
- settings,
- &glsl);
- if (!vs || !this->compileAndAttachShaders(glsl.c_str(), glsl.size(), programID,
- GR_GL_VERTEX_SHADER, &shadersToDelete, settings,
- inputs)) {
+ if (glsl.vs().empty()) {
+ // Don't have cached GLSL, need to compile SkSL->GLSL
+ std::unique_ptr<SkSL::Program> vs = GrSkSLtoGLSL(gpu()->glContext(),
+ GR_GL_VERTEX_SHADER,
+ fVS.fCompilerStrings.begin(),
+ fVS.fCompilerStringLengths.begin(),
+ fVS.fCompilerStrings.count(),
+ settings,
+ &glsl.vs());
+ if (!vs) {
+ this->cleanupProgram(programID, shadersToDelete);
+ return nullptr;
+ }
+ }
+ if (!this->compileAndAttachShaders(glsl.vs().c_str(), glsl.vs().size(), programID,
+ GR_GL_VERTEX_SHADER, &shadersToDelete, settings,
+ inputs)) {
this->cleanupProgram(programID, shadersToDelete);
return nullptr;
}
@@ -304,17 +365,24 @@
}
if (primProc.willUseGeoShader()) {
- std::unique_ptr<SkSL::Program> gs;
- gs = GrSkSLtoGLSL(gpu()->glContext(),
- GR_GL_GEOMETRY_SHADER,
- fGS.fCompilerStrings.begin(),
- fGS.fCompilerStringLengths.begin(),
- fGS.fCompilerStrings.count(),
- settings,
- &glsl);
- if (!gs || !this->compileAndAttachShaders(glsl.c_str(), glsl.size(), programID,
- GR_GL_GEOMETRY_SHADER, &shadersToDelete,
- settings, inputs)) {
+ if (glsl.gs().empty()) {
+ // Don't have cached GLSL, need to compile SkSL->GLSL
+ std::unique_ptr<SkSL::Program> gs;
+ gs = GrSkSLtoGLSL(gpu()->glContext(),
+ GR_GL_GEOMETRY_SHADER,
+ fGS.fCompilerStrings.begin(),
+ fGS.fCompilerStringLengths.begin(),
+ fGS.fCompilerStrings.count(),
+ settings,
+ &glsl.gs());
+ if (!gs) {
+ this->cleanupProgram(programID, shadersToDelete);
+ return nullptr;
+ }
+ }
+ if (!this->compileAndAttachShaders(glsl.gs().c_str(), glsl.gs().size(), programID,
+ GR_GL_GEOMETRY_SHADER, &shadersToDelete, settings,
+ inputs)) {
this->cleanupProgram(programID, shadersToDelete);
return nullptr;
}
diff --git a/src/gpu/gl/builders/GrGLProgramBuilder.h b/src/gpu/gl/builders/GrGLProgramBuilder.h
index 64b27b3..add3e51 100644
--- a/src/gpu/gl/builders/GrGLProgramBuilder.h
+++ b/src/gpu/gl/builders/GrGLProgramBuilder.h
@@ -20,6 +20,7 @@
class GrFragmentProcessor;
class GrGLContextInfo;
class GrProgramDesc;
+struct GrGLSLSet;
class GrGLSLShaderBuilder;
class GrShaderCaps;
@@ -63,7 +64,7 @@
void computeCountsAndStrides(GrGLuint programID, const GrPrimitiveProcessor& primProc,
bool bindAttribLocations);
void storeShaderInCache(const SkSL::Program::Inputs& inputs, GrGLuint programID,
- const SkSL::String& glsl);
+ const GrGLSLSet& glsl);
GrGLProgram* finalize();
void bindProgramResourceLocations(GrGLuint programID);
bool checkLinkStatus(GrGLuint programID);
diff --git a/tools/flags/CommonFlagsConfig.cpp b/tools/flags/CommonFlagsConfig.cpp
index c99aaa3..ee41b60 100644
--- a/tools/flags/CommonFlagsConfig.cpp
+++ b/tools/flags/CommonFlagsConfig.cpp
@@ -70,7 +70,8 @@
{ "gldft", "gpu", "api=gl,dit=true" },
{ "glesdft", "gpu", "api=gles,dit=true" },
{ "gltestthreading", "gpu", "api=gl,testThreading=true" },
- { "gltestpersistentcache", "gpu", "api=gl,testPersistentCache=true" },
+ { "gltestpersistentcache", "gpu", "api=gl,testPersistentCache=1" },
+ { "gltestglslcache", "gpu", "api=gl,testPersistentCache=2" },
{ "angle_d3d11_es2", "gpu", "api=angle_d3d11_es2" },
{ "angle_d3d11_es3", "gpu", "api=angle_d3d11_es3" },
{ "angle_d3d9_es2", "gpu", "api=angle_d3d9_es2" },
@@ -97,7 +98,7 @@
{ "vkmsaa8", "gpu", "api=vulkan,samples=8" },
{ "vkbetex", "gpu", "api=vulkan,surf=betex" },
{ "vkbert", "gpu", "api=vulkan,surf=bert" },
- { "vktestpersistentcache", "gpu", "api=vulkan,testPersistentCache=true" },
+ { "vktestpersistentcache", "gpu", "api=vulkan,testPersistentCache=1" },
#endif
#ifdef SK_METAL
{ "mtl", "gpu", "api=metal" },
@@ -168,8 +169,9 @@
"\t Allow the use of stencil buffers.\n"
"\ttestThreading\ttype: bool\tdefault: false.\n"
"\t Run with and without worker threads, check that results match.\n"
- "\ttestPersistentCache\ttype: bool\tdefault: false.\n"
- "\t Run using a pre-warmed GrContextOption::fPersistentCache.\n"
+ "\ttestPersistentCache\ttype: int\tdefault: 0.\n"
+ "\t 1: Run using a pre-warmed binary GrContextOptions::fPersistentCache.\n"
+ "\t 2: Run using a pre-warmed GLSL GrContextOptions::fPersistentCache.\n"
"\tsurf\ttype: string\tdefault: default.\n"
"\t Controls the type of backing store for SkSurfaces.\n"
"\t Options:\n"
@@ -437,7 +439,7 @@
sk_sp<SkColorSpace> colorSpace,
bool useStencilBuffers,
bool testThreading,
- bool testPersistentCache,
+ int testPersistentCache,
SurfType surfType)
: SkCommandLineConfig(tag, SkString("gpu"), viaParts)
, fContextType(contextType)
@@ -475,7 +477,7 @@
sk_sp<SkColorSpace> colorSpace = nullptr;
bool useStencils = true;
bool testThreading = false;
- bool testPersistentCache = false;
+ int testPersistentCache = 0;
SkCommandLineConfigGpu::SurfType surfType = SkCommandLineConfigGpu::SurfType::kDefault;
bool parseSucceeded = false;
@@ -492,11 +494,11 @@
extendedOptions.get_option_gpu_color("color", &colorType, &alphaType, &colorSpace) &&
extendedOptions.get_option_bool("stencils", &useStencils) &&
extendedOptions.get_option_bool("testThreading", &testThreading) &&
- extendedOptions.get_option_bool("testPersistentCache", &testPersistentCache) &&
+ extendedOptions.get_option_int("testPersistentCache", &testPersistentCache) &&
extendedOptions.get_option_gpu_surf_type("surf", &surfType);
// testing threading and the persistent cache are mutually exclusive.
- if (!validOptions || (testThreading && testPersistentCache)) {
+ if (!validOptions || (testThreading && (testPersistentCache != 0))) {
return nullptr;
}
diff --git a/tools/flags/CommonFlagsConfig.h b/tools/flags/CommonFlagsConfig.h
index 1e9db26..63b18ce 100644
--- a/tools/flags/CommonFlagsConfig.h
+++ b/tools/flags/CommonFlagsConfig.h
@@ -62,7 +62,7 @@
sk_sp<SkColorSpace> colorSpace,
bool useStencilBuffers,
bool testThreading,
- bool testPersistentCache,
+ int testPersistentCache,
SurfType);
const SkCommandLineConfigGpu* asConfigGpu() const override { return this; }
@@ -79,7 +79,7 @@
SkAlphaType getAlphaType() const { return fAlphaType; }
SkColorSpace* getColorSpace() const { return fColorSpace.get(); }
bool getTestThreading() const { return fTestThreading; }
- bool getTestPersistentCache() const { return fTestPersistentCache; }
+ int getTestPersistentCache() const { return fTestPersistentCache; }
SurfType getSurfType() const { return fSurfType; }
private:
@@ -91,7 +91,7 @@
SkAlphaType fAlphaType;
sk_sp<SkColorSpace> fColorSpace;
bool fTestThreading;
- bool fTestPersistentCache;
+ int fTestPersistentCache;
SurfType fSurfType;
};