Add mutex to guard GrSkSLFPFactoryCache accesses
In the DDL-recording world we're now computing GrProgramDescs at record time. This ends up creating the GrSkSLFPFactories involved in multiple threads. As part of the GrContextThreadSafeProxy, the GrSkSLFPFactoryCache has to be thread safe.
This CL also forces GrSkSLFP.h and GrSkSLFPFactoryCache.h to only ever be included in .cpp files.
Bug: skia:9455
Change-Id: Ic5e87c28d2c1c5b468ac81e07b66c1191efbbd65
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/257634
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
diff --git a/src/core/SkColorFilter.cpp b/src/core/SkColorFilter.cpp
index 8920bde..6db5e45 100644
--- a/src/core/SkColorFilter.cpp
+++ b/src/core/SkColorFilter.cpp
@@ -22,6 +22,7 @@
#if SK_SUPPORT_GPU
#include "src/gpu/GrFragmentProcessor.h"
+#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/gpu/effects/generated/GrMixerEffect.h"
#endif
diff --git a/src/effects/SkOverdrawColorFilter.cpp b/src/effects/SkOverdrawColorFilter.cpp
index c91748e..81e2ce4 100644
--- a/src/effects/SkOverdrawColorFilter.cpp
+++ b/src/effects/SkOverdrawColorFilter.cpp
@@ -12,6 +12,7 @@
#include "src/core/SkReadBuffer.h"
#if SK_SUPPORT_GPU
+#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/gpu/effects/GrSkSLFP.h"
GR_FP_SRC_STRING SKSL_OVERDRAW_SRC = R"(
diff --git a/src/effects/imagefilters/SkArithmeticImageFilter.cpp b/src/effects/imagefilters/SkArithmeticImageFilter.cpp
index 42c62be..7b3deb8 100644
--- a/src/effects/imagefilters/SkArithmeticImageFilter.cpp
+++ b/src/effects/imagefilters/SkArithmeticImageFilter.cpp
@@ -21,6 +21,7 @@
#include "src/gpu/GrColorSpaceXform.h"
#include "src/gpu/GrRecordingContextPriv.h"
#include "src/gpu/GrRenderTargetContext.h"
+#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/gpu/GrTextureProxy.h"
#include "src/gpu/SkGr.h"
#include "src/gpu/effects/GrSkSLFP.h"
diff --git a/src/gpu/GrCaps.h b/src/gpu/GrCaps.h
index 3c5501d..49fdf70 100644
--- a/src/gpu/GrCaps.h
+++ b/src/gpu/GrCaps.h
@@ -39,6 +39,7 @@
void dumpJSON(SkJSONWriter*) const;
const GrShaderCaps* shaderCaps() const { return fShaderCaps.get(); }
+ sk_sp<const GrShaderCaps> refShaderCaps() const { return fShaderCaps; }
bool npotTextureTileSupport() const { return fNPOTTextureTileSupport; }
/** To avoid as-yet-unnecessary complexity we don't allow any partial support of MIP Maps (e.g.
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 6a5eb84..0ab948c 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -26,6 +26,7 @@
#include "src/gpu/GrResourceProvider.h"
#include "src/gpu/GrSemaphore.h"
#include "src/gpu/GrShaderUtils.h"
+#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/gpu/GrSoftwarePathRenderer.h"
#include "src/gpu/GrTracing.h"
#include "src/gpu/SkGr.h"
diff --git a/src/gpu/GrContextPriv.cpp b/src/gpu/GrContextPriv.cpp
index a84ace6..4a4bd3c 100644
--- a/src/gpu/GrContextPriv.cpp
+++ b/src/gpu/GrContextPriv.cpp
@@ -20,6 +20,7 @@
#include "src/gpu/GrSurfacePriv.h"
#include "src/gpu/GrTextureContext.h"
#include "src/gpu/SkGr.h"
+#include "src/gpu/effects/GrSkSLFP.h"
#include "src/gpu/effects/generated/GrConfigConversionEffect.h"
#include "src/gpu/text/GrTextBlobCache.h"
#include "src/image/SkImage_Base.h"
diff --git a/src/gpu/GrContextThreadSafeProxy.cpp b/src/gpu/GrContextThreadSafeProxy.cpp
index 64c7bf6..6fe042d 100644
--- a/src/gpu/GrContextThreadSafeProxy.cpp
+++ b/src/gpu/GrContextThreadSafeProxy.cpp
@@ -13,6 +13,7 @@
#include "src/gpu/GrBaseContextPriv.h"
#include "src/gpu/GrCaps.h"
#include "src/gpu/GrSkSLFPFactoryCache.h"
+#include "src/gpu/effects/GrSkSLFP.h"
#include "src/image/SkSurface_Gpu.h"
#ifdef SK_VULKAN
diff --git a/src/gpu/GrContext_Base.cpp b/src/gpu/GrContext_Base.cpp
index 1c79b32..f7d3f77 100644
--- a/src/gpu/GrContext_Base.cpp
+++ b/src/gpu/GrContext_Base.cpp
@@ -10,6 +10,7 @@
#include "src/gpu/GrBaseContextPriv.h"
#include "src/gpu/GrCaps.h"
#include "src/gpu/GrSkSLFPFactoryCache.h"
+#include "src/gpu/effects/GrSkSLFP.h"
static int32_t next_id() {
static std::atomic<int32_t> nextID{1};
diff --git a/src/gpu/GrDDLContext.cpp b/src/gpu/GrDDLContext.cpp
index 1e4a17f..616f7c4 100644
--- a/src/gpu/GrDDLContext.cpp
+++ b/src/gpu/GrDDLContext.cpp
@@ -10,6 +10,7 @@
#include "src/gpu/GrContextPriv.h"
#include "src/gpu/GrContextThreadSafeProxyPriv.h"
#include "src/gpu/GrSkSLFPFactoryCache.h"
+#include "src/gpu/effects/GrSkSLFP.h"
/**
* The DDL Context is the one in effect during DDL Recording. It isn't backed by a GrGPU and
diff --git a/src/gpu/GrDeferredUpload.h b/src/gpu/GrDeferredUpload.h
index 887f853..946787b 100644
--- a/src/gpu/GrDeferredUpload.h
+++ b/src/gpu/GrDeferredUpload.h
@@ -25,8 +25,8 @@
* Ops, in conjunction with helpers such as GrDrawOpAtlas, use upload tokens to know what the most
* recent draw was that referenced a resource (or portion of a resource). Each draw is assigned a
* token. A resource (or portion thereof) can be tagged with the most recent reading draw's token.
- * The deferred uploads target provides a facility for testing whether the draw corresponding to the
- * token has been flushed. If it has not been flushed then the op must perform an inline upload
+ * The deferred upload's target provides a facility for testing whether the draw corresponding to
+ * the token has been flushed. If it has not been flushed then the op must perform an inline upload
* instead so that the upload occurs after the draw depending on the old contents and before the
* draw depending on the updated contents. When scheduling an inline upload the op provides the
* token of the draw that the upload must occur before.
diff --git a/src/gpu/GrImageContext.cpp b/src/gpu/GrImageContext.cpp
index db4c9cb..39256fc 100644
--- a/src/gpu/GrImageContext.cpp
+++ b/src/gpu/GrImageContext.cpp
@@ -11,6 +11,7 @@
#include "src/gpu/GrImageContextPriv.h"
#include "src/gpu/GrProxyProvider.h"
#include "src/gpu/GrSkSLFPFactoryCache.h"
+#include "src/gpu/effects/GrSkSLFP.h"
#define ASSERT_SINGLE_OWNER \
SkDEBUGCODE(GrSingleOwner::AutoEnforce debug_SingleOwner(this->singleOwner());)
diff --git a/src/gpu/GrLegacyDirectContext.cpp b/src/gpu/GrLegacyDirectContext.cpp
index 34002a8..d380eae 100644
--- a/src/gpu/GrLegacyDirectContext.cpp
+++ b/src/gpu/GrLegacyDirectContext.cpp
@@ -12,6 +12,7 @@
#include "src/gpu/GrContextPriv.h"
#include "src/gpu/GrContextThreadSafeProxyPriv.h"
#include "src/gpu/GrGpu.h"
+#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/gpu/effects/GrSkSLFP.h"
#include "src/gpu/gl/GrGLGpu.h"
@@ -72,7 +73,7 @@
SkASSERT(caps && !FPFactoryCache);
SkASSERT(!fThreadSafeProxy);
- FPFactoryCache.reset(new GrSkSLFPFactoryCache());
+ FPFactoryCache.reset(new GrSkSLFPFactoryCache(caps->refShaderCaps()));
fThreadSafeProxy = GrContextThreadSafeProxyPriv::Make(this->backend(),
this->options(),
this->contextID(),
diff --git a/src/gpu/GrRecordingContext.cpp b/src/gpu/GrRecordingContext.cpp
index b7f9d46..18a1114 100644
--- a/src/gpu/GrRecordingContext.cpp
+++ b/src/gpu/GrRecordingContext.cpp
@@ -19,6 +19,7 @@
#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/gpu/GrTextureContext.h"
#include "src/gpu/SkGr.h"
+#include "src/gpu/effects/GrSkSLFP.h"
#include "src/gpu/text/GrTextBlobCache.h"
#define ASSERT_SINGLE_OWNER_PRIV \
diff --git a/src/gpu/GrSkSLFPFactoryCache.h b/src/gpu/GrSkSLFPFactoryCache.h
index 48869f8..180fcb0 100644
--- a/src/gpu/GrSkSLFPFactoryCache.h
+++ b/src/gpu/GrSkSLFPFactoryCache.h
@@ -8,9 +8,9 @@
#ifndef GrSkSLFPFactoryCache_DEFINED
#define GrSkSLFPFactoryCache_DEFINED
-#include "include/core/SkRefCnt.h"
-
#include <vector>
+#include "include/core/SkRefCnt.h"
+#include "src/sksl/ir/SkSLProgram.h"
class GrSkSLFPFactory;
@@ -21,6 +21,13 @@
// onGetGLSLProcessorKey.
class GrSkSLFPFactoryCache : public SkNVRefCnt<GrSkSLFPFactoryCache> {
public:
+ GrSkSLFPFactoryCache(sk_sp<const GrShaderCaps> shaderCaps) : fShaderCaps(shaderCaps) {}
+ ~GrSkSLFPFactoryCache();
+
+ sk_sp<GrSkSLFPFactory> findOrCreate(int index, const char* name, const char* skSL,
+ SkSL::Program::Kind kind);
+
+private:
// Returns a factory by its numeric index, or null if no such factory exists. Indices are
// allocated by GrSkSLFP::NewIndex().
sk_sp<GrSkSLFPFactory> get(int index);
@@ -28,10 +35,10 @@
// Stores a new factory with the given index.
void set(int index, sk_sp<GrSkSLFPFactory> factory);
- ~GrSkSLFPFactoryCache();
+ mutable SkMutex fCacheMutex;
-private:
- std::vector<GrSkSLFPFactory*> fFactories;
+ std::vector<sk_sp<GrSkSLFPFactory>> fFactories;
+ sk_sp<const GrShaderCaps> fShaderCaps;
};
#endif
diff --git a/src/gpu/SkGr.cpp b/src/gpu/SkGr.cpp
index 0472ee2..cd560f1 100644
--- a/src/gpu/SkGr.cpp
+++ b/src/gpu/SkGr.cpp
@@ -34,6 +34,7 @@
#include "src/gpu/GrPaint.h"
#include "src/gpu/GrProxyProvider.h"
#include "src/gpu/GrRecordingContextPriv.h"
+#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/gpu/GrTextureProxy.h"
#include "src/gpu/GrXferProcessor.h"
#include "src/gpu/effects/GrBicubicEffect.h"
diff --git a/src/gpu/effects/GrSkSLFP.cpp b/src/gpu/effects/GrSkSLFP.cpp
index df65774..b856fa4 100644
--- a/src/gpu/effects/GrSkSLFP.cpp
+++ b/src/gpu/effects/GrSkSLFP.cpp
@@ -10,6 +10,7 @@
#include "include/gpu/GrTexture.h"
#include "include/private/GrContext_Base.h"
#include "src/gpu/GrBaseContextPriv.h"
+#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/sksl/SkSLUtil.h"
#include "src/sksl/ir/SkSLVarDeclarations.h"
@@ -369,7 +370,6 @@
size_t inputSize, SkSL::Program::Kind kind,
const SkMatrix* matrix) {
return std::unique_ptr<GrSkSLFP>(new GrSkSLFP(context->priv().fpFactoryCache(),
- context->priv().caps()->shaderCaps(),
kind, index, name, sksl, SkString(),
inputs, inputSize, matrix));
}
@@ -378,18 +378,16 @@
SkString sksl, const void* inputs, size_t inputSize,
SkSL::Program::Kind kind, const SkMatrix* matrix) {
return std::unique_ptr<GrSkSLFP>(new GrSkSLFP(context->priv().fpFactoryCache(),
- context->priv().caps()->shaderCaps(),
kind, index, name, nullptr, std::move(sksl),
inputs, inputSize, matrix));
}
-GrSkSLFP::GrSkSLFP(sk_sp<GrSkSLFPFactoryCache> factoryCache, const GrShaderCaps* shaderCaps,
+GrSkSLFP::GrSkSLFP(sk_sp<GrSkSLFPFactoryCache> factoryCache,
SkSL::Program::Kind kind, int index, const char* name, const char* sksl,
SkString skslString, const void* inputs, size_t inputSize,
const SkMatrix* matrix)
: INHERITED(kGrSkSLFP_ClassID, kNone_OptimizationFlags)
, fFactoryCache(factoryCache)
- , fShaderCaps(sk_ref_sp(shaderCaps))
, fKind(kind)
, fIndex(index)
, fName(name)
@@ -409,7 +407,6 @@
GrSkSLFP::GrSkSLFP(const GrSkSLFP& other)
: INHERITED(kGrSkSLFP_ClassID, kNone_OptimizationFlags)
, fFactoryCache(other.fFactoryCache)
- , fShaderCaps(other.fShaderCaps)
, fFactory(other.fFactory)
, fKind(other.fKind)
, fIndex(other.fIndex)
@@ -433,12 +430,7 @@
void GrSkSLFP::createFactory() const {
if (!fFactory) {
- fFactory = fFactoryCache->get(fIndex);
- if (!fFactory) {
- fFactory = sk_sp<GrSkSLFPFactory>(new GrSkSLFPFactory(fName, fShaderCaps.get(), fSkSL,
- fKind));
- fFactoryCache->set(fIndex, fFactory);
- }
+ fFactory = fFactoryCache->findOrCreate(fIndex, fName, fSkSL, fKind);
}
}
@@ -553,35 +545,39 @@
return std::unique_ptr<GrFragmentProcessor>(result.release());
}
-// We have to do a bit of manual refcounting in the cache methods below. Ideally, we could just
-// define fFactories to contain sk_sp<GrSkSLFPFactory> rather than GrSkSLFPFactory*, but that would
-// require GrContext to include GrSkSLFP, which creates much bigger headaches than a few manual
-// refcounts.
+/**************************************************************************************************/
+GrSkSLFPFactoryCache::~GrSkSLFPFactoryCache() {}
+
+sk_sp<GrSkSLFPFactory> GrSkSLFPFactoryCache::findOrCreate(int index, const char* name,
+ const char* skSL,
+ SkSL::Program::Kind kind) {
+ // acquire lock for checking/adding to cache
+ SkAutoMutexExclusive ame(fCacheMutex);
+
+ sk_sp<GrSkSLFPFactory> factory = this->get(index);
+ if (!factory) {
+ factory = sk_sp<GrSkSLFPFactory>(new GrSkSLFPFactory(name, fShaderCaps.get(),
+ skSL, kind));
+ this->set(index, factory);
+ }
+
+ return factory;
+}
sk_sp<GrSkSLFPFactory> GrSkSLFPFactoryCache::get(int index) {
if (index >= (int) fFactories.size()) {
return nullptr;
}
- GrSkSLFPFactory* result = fFactories[index];
- SkSafeRef(result);
- return sk_sp<GrSkSLFPFactory>(result);
+
+ return fFactories[index];
}
void GrSkSLFPFactoryCache::set(int index, sk_sp<GrSkSLFPFactory> factory) {
while (index >= (int) fFactories.size()) {
fFactories.emplace_back();
}
- factory->ref();
SkASSERT(!fFactories[index]);
- fFactories[index] = factory.get();
-}
-
-GrSkSLFPFactoryCache::~GrSkSLFPFactoryCache() {
- for (GrSkSLFPFactory* factory : fFactories) {
- if (factory) {
- factory->unref();
- }
- }
+ fFactories[index] = std::move(factory);
}
GR_DEFINE_FRAGMENT_PROCESSOR_TEST(GrSkSLFP);
diff --git a/src/gpu/effects/GrSkSLFP.h b/src/gpu/effects/GrSkSLFP.h
index eba33c6..a5013b3 100644
--- a/src/gpu/effects/GrSkSLFP.h
+++ b/src/gpu/effects/GrSkSLFP.h
@@ -12,8 +12,6 @@
#include "src/gpu/GrCaps.h"
#include "src/gpu/GrCoordTransform.h"
#include "src/gpu/GrFragmentProcessor.h"
-#include "src/gpu/GrShaderCaps.h"
-#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/sksl/SkSLCompiler.h"
#include "src/sksl/SkSLPipelineStageCodeGenerator.h"
#include <atomic>
@@ -25,7 +23,9 @@
#endif
class GrContext_Base;
+class GrShaderCaps;
class GrSkSLFPFactory;
+class GrSkSLFPFactoryCache;
class GrSkSLFP : public GrFragmentProcessor {
public:
@@ -112,7 +112,7 @@
std::unique_ptr<GrFragmentProcessor> clone() const override;
private:
- GrSkSLFP(sk_sp<GrSkSLFPFactoryCache> factoryCache, const GrShaderCaps* shaderCaps,
+ GrSkSLFP(sk_sp<GrSkSLFPFactoryCache> factoryCache,
SkSL::Program::Kind kind, int fIndex, const char* name, const char* sksl,
SkString skslString, const void* inputs, size_t inputSize, const SkMatrix* matrix);
@@ -128,8 +128,6 @@
sk_sp<GrSkSLFPFactoryCache> fFactoryCache;
- const sk_sp<GrShaderCaps> fShaderCaps;
-
mutable sk_sp<GrSkSLFPFactory> fFactory;
SkSL::Program::Kind fKind;
diff --git a/src/shaders/SkRTShader.cpp b/src/shaders/SkRTShader.cpp
index c42a8d0..95cffe5 100644
--- a/src/shaders/SkRTShader.cpp
+++ b/src/shaders/SkRTShader.cpp
@@ -20,6 +20,7 @@
#include "src/gpu/GrCaps.h"
#include "src/gpu/GrColorInfo.h"
#include "src/gpu/GrRecordingContextPriv.h"
+#include "src/gpu/GrSkSLFPFactoryCache.h"
#include "src/gpu/SkGr.h"
#include "src/gpu/GrFragmentProcessor.h"