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