VkPipelineCache: Do not publically expose internal mutexes
They make it unnecessarily easy to write code that violates the mutex lock requirements.
Replace with the `getOrCreate` pattern already used in vk::Device, which keeps the locking internal.
This change fixes a couple of places that we were accesssing data without correct locking (real bugs).
Bug: b/153194656
Change-Id: I132c450594f4042160b575197789bca7f1a5e25f
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/43650
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Ben Clayton <bclayton@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/Vulkan/VkDevice.hpp b/src/Vulkan/VkDevice.hpp
index 3ba442d..8931949 100644
--- a/src/Vulkan/VkDevice.hpp
+++ b/src/Vulkan/VkDevice.hpp
@@ -86,8 +86,14 @@
};
};
+ // getOrCreate() queries the cache for a Routine with the given key.
+ // If one is found, it is returned, otherwise createRoutine(key) is
+ // called, the returned Routine is added to the cache, and it is
+ // returned.
+ // Function must be a function of the signature:
+ // std::shared_ptr<rr::Routine>(const Key &)
template<typename Function>
- std::shared_ptr<rr::Routine> getOrCreate(const Key &key, Function createRoutine)
+ std::shared_ptr<rr::Routine> getOrCreate(const Key &key, Function &&createRoutine)
{
std::lock_guard<std::mutex> lock(mutex);
diff --git a/src/Vulkan/VkPipeline.cpp b/src/Vulkan/VkPipeline.cpp
index e8182b2..0de27b2 100644
--- a/src/Vulkan/VkPipeline.cpp
+++ b/src/Vulkan/VkPipeline.cpp
@@ -483,21 +483,10 @@
if(pPipelineCache)
{
- PipelineCache &pipelineCache = *pPipelineCache;
- {
- std::unique_lock<std::mutex> lock(pipelineCache.getShaderMutex());
- const std::shared_ptr<sw::SpirvShader> *spirvShader = pipelineCache[key];
- if(!spirvShader)
- {
- auto shader = createShader(key, module, robustBufferAccess, device->getDebuggerContext());
- setShader(pipelineStage, shader);
- pipelineCache.insert(key, getShader(pipelineStage));
- }
- else
- {
- setShader(pipelineStage, *spirvShader);
- }
- }
+ auto shader = pPipelineCache->getOrCreateShader(key, [&] {
+ return createShader(key, module, robustBufferAccess, device->getDebuggerContext());
+ });
+ setShader(pipelineStage, shader);
}
else
{
@@ -583,35 +572,14 @@
stage.stage, stage.pName, module->getCode(), nullptr, 0, stage.pSpecializationInfo);
if(pPipelineCache)
{
- PipelineCache &pipelineCache = *pPipelineCache;
- {
- std::unique_lock<std::mutex> lock(pipelineCache.getShaderMutex());
- const std::shared_ptr<sw::SpirvShader> *spirvShader = pipelineCache[shaderKey];
- if(!spirvShader)
- {
- shader = createShader(shaderKey, module, robustBufferAccess, device->getDebuggerContext());
- pipelineCache.insert(shaderKey, shader);
- }
- else
- {
- shader = *spirvShader;
- }
- }
+ shader = pPipelineCache->getOrCreateShader(shaderKey, [&] {
+ return createShader(shaderKey, module, robustBufferAccess, device->getDebuggerContext());
+ });
- {
- const PipelineCache::ComputeProgramKey programKey(shader.get(), layout);
- std::unique_lock<std::mutex> lock(pipelineCache.getProgramMutex());
- const std::shared_ptr<sw::ComputeProgram> *computeProgram = pipelineCache[programKey];
- if(!computeProgram)
- {
- program = createProgram(programKey);
- pipelineCache.insert(programKey, program);
- }
- else
- {
- program = *computeProgram;
- }
- }
+ const PipelineCache::ComputeProgramKey programKey(shader.get(), layout);
+ program = pPipelineCache->getOrCreateComputeProgram(programKey, [&] {
+ return createProgram(programKey);
+ });
}
else
{
diff --git a/src/Vulkan/VkPipelineCache.cpp b/src/Vulkan/VkPipelineCache.cpp
index f54ac90..c725b3f 100644
--- a/src/Vulkan/VkPipelineCache.cpp
+++ b/src/Vulkan/VkPipelineCache.cpp
@@ -224,12 +224,14 @@
PipelineCache *srcCache = Cast(pSrcCaches[i]);
{
- std::unique_lock<std::mutex> lock(spirvShadersMutex);
+ std::unique_lock<std::mutex> thisLock(spirvShadersMutex);
+ std::unique_lock<std::mutex> srcLock(srcCache->spirvShadersMutex);
spirvShaders.insert(srcCache->spirvShaders.begin(), srcCache->spirvShaders.end());
}
{
- std::unique_lock<std::mutex> lock(computeProgramsMutex);
+ std::unique_lock<std::mutex> thisLock(computeProgramsMutex);
+ std::unique_lock<std::mutex> srcLock(srcCache->computeProgramsMutex);
computePrograms.insert(srcCache->computePrograms.begin(), srcCache->computePrograms.end());
}
}
@@ -237,26 +239,4 @@
return VK_SUCCESS;
}
-const std::shared_ptr<sw::SpirvShader> *PipelineCache::operator[](const PipelineCache::SpirvShaderKey &key) const
-{
- auto it = spirvShaders.find(key);
- return (it != spirvShaders.end()) ? &(it->second) : nullptr;
-}
-
-void PipelineCache::insert(const PipelineCache::SpirvShaderKey &key, const std::shared_ptr<sw::SpirvShader> &shader)
-{
- spirvShaders[key] = shader;
-}
-
-const std::shared_ptr<sw::ComputeProgram> *PipelineCache::operator[](const PipelineCache::ComputeProgramKey &key) const
-{
- auto it = computePrograms.find(key);
- return (it != computePrograms.end()) ? &(it->second) : nullptr;
-}
-
-void PipelineCache::insert(const PipelineCache::ComputeProgramKey &key, const std::shared_ptr<sw::ComputeProgram> &computeProgram)
-{
- computePrograms[key] = computeProgram;
-}
-
} // namespace vk
diff --git a/src/Vulkan/VkPipelineCache.hpp b/src/Vulkan/VkPipelineCache.hpp
index 42b4df6..85c5836 100644
--- a/src/Vulkan/VkPipelineCache.hpp
+++ b/src/Vulkan/VkPipelineCache.hpp
@@ -93,9 +93,13 @@
const SpecializationInfo specializationInfo;
};
- std::mutex &getShaderMutex() { return spirvShadersMutex; }
- const std::shared_ptr<sw::SpirvShader> *operator[](const PipelineCache::SpirvShaderKey &key) const;
- void insert(const PipelineCache::SpirvShaderKey &key, const std::shared_ptr<sw::SpirvShader> &shader);
+ // getOrCreateShader() queries the cache for a shader with the given key.
+ // If one is found, it is returned, otherwise create() is called, the
+ // returned shader is added to the cache, and it is returned.
+ // Function must be a function of the signature:
+ // std::shared_ptr<sw::SpirvShader>()
+ template<typename Function>
+ inline std::shared_ptr<sw::SpirvShader> getOrCreateShader(const PipelineCache::SpirvShaderKey &key, Function &&create);
struct ComputeProgramKey
{
@@ -117,9 +121,14 @@
const vk::PipelineLayout *layout;
};
- std::mutex &getProgramMutex() { return computeProgramsMutex; }
- const std::shared_ptr<sw::ComputeProgram> *operator[](const PipelineCache::ComputeProgramKey &key) const;
- void insert(const PipelineCache::ComputeProgramKey &key, const std::shared_ptr<sw::ComputeProgram> &computeProgram);
+ // getOrCreateComputeProgram() queries the cache for a compute program with
+ // the given key.
+ // If one is found, it is returned, otherwise create() is called, the
+ // returned program is added to the cache, and it is returned.
+ // Function must be a function of the signature:
+ // std::shared_ptr<sw::ComputeProgram>()
+ template<typename Function>
+ inline std::shared_ptr<sw::ComputeProgram> getOrCreateComputeProgram(const PipelineCache::ComputeProgramKey &key, Function &&create);
private:
struct CacheHeader
@@ -135,10 +144,10 @@
uint8_t *data = nullptr;
std::mutex spirvShadersMutex;
- std::map<SpirvShaderKey, std::shared_ptr<sw::SpirvShader>> spirvShaders;
+ std::map<SpirvShaderKey, std::shared_ptr<sw::SpirvShader>> spirvShaders; // guarded by spirvShadersMutex
std::mutex computeProgramsMutex;
- std::map<ComputeProgramKey, std::shared_ptr<sw::ComputeProgram>> computePrograms;
+ std::map<ComputeProgramKey, std::shared_ptr<sw::ComputeProgram>> computePrograms; // guarded by computeProgramsMutex
};
static inline PipelineCache *Cast(VkPipelineCache object)
@@ -146,6 +155,32 @@
return PipelineCache::Cast(object);
}
+template<typename Function>
+std::shared_ptr<sw::ComputeProgram> PipelineCache::getOrCreateComputeProgram(const PipelineCache::ComputeProgramKey &key, Function &&create)
+{
+ std::unique_lock<std::mutex> lock(computeProgramsMutex);
+
+ auto it = computePrograms.find(key);
+ if(it != computePrograms.end()) { return it->second; }
+
+ auto created = create();
+ computePrograms.emplace(key, created);
+ return created;
+}
+
+template<typename Function>
+std::shared_ptr<sw::SpirvShader> PipelineCache::getOrCreateShader(const PipelineCache::SpirvShaderKey &key, Function &&create)
+{
+ std::unique_lock<std::mutex> lock(spirvShadersMutex);
+
+ auto it = spirvShaders.find(key);
+ if(it != spirvShaders.end()) { return it->second; }
+
+ auto created = create();
+ spirvShaders.emplace(key, created);
+ return created;
+}
+
} // namespace vk
#endif // VK_PIPELINE_CACHE_HPP_
