From 945e2642d6f77df04d1b0c79e898bd960a6266bd Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Wed, 14 Sep 2022 18:13:17 +0000 Subject: [PATCH] Fix race with FencedDeleter and VkShaderModule labels in async creation FencedDeleter does not need to be used at all since we were deleting a yet-to-be-used VkShaderModule. Also, set the VkShaderModule label before cache deduplication since to avoid a race where the VkShaderModule is in use by another thread. The other thread may also be setting the label, or using it in a pipeline creation. Bug: dawn:1539 Change-Id: I5e3d7ce214c4c089c9cc3272f373aa8233017965 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/102105 Commit-Queue: Austin Eng Kokoro: Kokoro Reviewed-by: Loko Kung --- src/dawn/native/vulkan/ShaderModuleVk.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/dawn/native/vulkan/ShaderModuleVk.cpp b/src/dawn/native/vulkan/ShaderModuleVk.cpp index 0ec0092414..752338e904 100644 --- a/src/dawn/native/vulkan/ShaderModuleVk.cpp +++ b/src/dawn/native/vulkan/ShaderModuleVk.cpp @@ -137,12 +137,16 @@ ShaderModule::ModuleAndSpirv ShaderModule::ConcurrentTransformedShaderModuleCach std::lock_guard lock(mMutex); auto iter = mTransformedShaderModuleCache.find(key); if (iter == mTransformedShaderModuleCache.end()) { - mTransformedShaderModuleCache.emplace(key, std::make_pair(module, std::move(spirv))); + bool added = false; + std::tie(iter, added) = + mTransformedShaderModuleCache.emplace(key, std::make_pair(module, std::move(spirv))); + ASSERT(added); } else { - mDevice->GetFencedDeleter()->DeleteWhenUnused(module); + // No need to use FencedDeleter since this shader module was just created and does + // not need to wait for queue operations to complete. + // Also, use of fenced deleter here is not thread safe. + mDevice->fn.DestroyShaderModule(mDevice->GetVkDevice(), module, nullptr); } - // Now the key should exist in the map, so find it again and return it. - iter = mTransformedShaderModuleCache.find(key); return ModuleAndSpirv{ iter->second.first, iter->second.second.Code(), @@ -371,12 +375,13 @@ ResultOrError ShaderModule::GetHandleAndSpirv( if (BlobCache* cache = device->GetBlobCache()) { cache->EnsureStored(spirv); } + // Set the label on `newHandle` now, and not on `moduleAndSpirv.module` later + // since `moduleAndSpirv.module` may be in use by multiple threads. + SetDebugName(ToBackend(GetDevice()), newHandle, "Dawn_ShaderModule", GetLabel()); moduleAndSpirv = mTransformedShaderModuleCache->AddOrGet(cacheKey, newHandle, spirv.Acquire()); } - SetDebugName(ToBackend(GetDevice()), moduleAndSpirv.module, "Dawn_ShaderModule", GetLabel()); - return std::move(moduleAndSpirv); #else return DAWN_INTERNAL_ERROR("TINT_BUILD_SPV_WRITER is not defined.");