From 7e788ec4e49b4cb0aad8a09c3b683ae7631b0a1a Mon Sep 17 00:00:00 2001 From: Jeroen Bakker Date: Tue, 17 Dec 2024 12:43:20 +0100 Subject: [PATCH] Fix #130942: Vulkan: Hash collision in graphics pipeline pool Blender stores all pipelines in a pool. Using a hash it checks if a the pipeline was already created and the previous could be reused. Due to performance issues when working with graphics pipelines some equal operations only used a hash check. For scissors and viewports this isn't enough and could lead to issues. This PR fixes this to still perform an exact check if the hash are equal. Note that the performance drops a bit. And should be countered with other performance improvements in the future. Pull Request: https://projects.blender.org/blender/blender/pulls/132005 --- source/blender/gpu/CMakeLists.txt | 1 + source/blender/gpu/vulkan/vk_pipeline_pool.hh | 87 +++++++++++-------- 2 files changed, 53 insertions(+), 35 deletions(-) diff --git a/source/blender/gpu/CMakeLists.txt b/source/blender/gpu/CMakeLists.txt index 2637094f8f1..2fefe5958af 100644 --- a/source/blender/gpu/CMakeLists.txt +++ b/source/blender/gpu/CMakeLists.txt @@ -437,6 +437,7 @@ if(WITH_VULKAN_BACKEND) ${VULKAN_LIBRARIES} ${SHADERC_LIBRARIES} extern_vulkan_memory_allocator + PRIVATE bf::extern::xxhash ) add_definitions(-DWITH_VULKAN_BACKEND) diff --git a/source/blender/gpu/vulkan/vk_pipeline_pool.hh b/source/blender/gpu/vulkan/vk_pipeline_pool.hh index cd2a74fe4d1..7d1503db5e5 100644 --- a/source/blender/gpu/vulkan/vk_pipeline_pool.hh +++ b/source/blender/gpu/vulkan/vk_pipeline_pool.hh @@ -10,6 +10,8 @@ #include +#include "xxhash.h" + #include "BLI_map.hh" #include "BLI_utility_mixins.hh" @@ -61,19 +63,12 @@ struct VKGraphicsInfo { uint64_t hash() const { - uint64_t hash = 0; - hash = hash * 33 ^ uint64_t(vk_topology); - for (const VkVertexInputAttributeDescription &attribute : attributes) { - hash = hash * 33 ^ uint64_t(attribute.location); - hash = hash * 33 ^ uint64_t(attribute.binding); - hash = hash * 33 ^ uint64_t(attribute.format); - hash = hash * 33 ^ uint64_t(attribute.offset); - } - for (const VkVertexInputBindingDescription &binding : bindings) { - hash = hash * 33 ^ uint64_t(binding.binding); - hash = hash * 33 ^ uint64_t(binding.inputRate); - hash = hash * 33 ^ uint64_t(binding.stride); - } + uint64_t hash = uint64_t(vk_topology); + hash = hash * 33 ^ + XXH3_64bits(attributes.data(), + attributes.size() * sizeof(VkVertexInputAttributeDescription)); + hash = hash * 33 ^ XXH3_64bits(bindings.data(), + bindings.size() * sizeof(VkVertexInputBindingDescription)); return hash; } }; @@ -101,8 +96,26 @@ struct VKGraphicsInfo { bool operator==(const FragmentShader &other) const { - /* TODO: Do not use hash. */ - return vk_fragment_module == other.vk_fragment_module && hash() == other.hash(); + if (vk_fragment_module != other.vk_fragment_module || + viewports.size() != other.viewports.size() || scissors.size() != other.scissors.size() || + hash() != other.hash()) + { + return false; + } + + if (memcmp(viewports.data(), + other.viewports.data(), + viewports.size() * sizeof(VkViewport)) != 0) + { + return false; + } + + if (memcmp(scissors.data(), other.scissors.data(), scissors.size() * sizeof(VkRect2D)) != 0) + { + return false; + } + + return true; } uint64_t hash() const @@ -121,22 +134,10 @@ struct VKGraphicsInfo { private: uint64_t calc_hash() const { - uint64_t hash = 0; - hash = hash * 33 ^ uint64_t(vk_fragment_module); - for (const VkViewport &vk_viewport : viewports) { - hash = hash * 33 ^ uint64_t(vk_viewport.x); - hash = hash * 33 ^ uint64_t(vk_viewport.y); - hash = hash * 33 ^ uint64_t(vk_viewport.width); - hash = hash * 33 ^ uint64_t(vk_viewport.height); - hash = hash * 33 ^ uint64_t(vk_viewport.minDepth); - hash = hash * 33 ^ uint64_t(vk_viewport.maxDepth); - } - for (const VkRect2D &scissor : scissors) { - hash = hash * 33 ^ uint64_t(scissor.offset.x); - hash = hash * 33 ^ uint64_t(scissor.offset.y); - hash = hash * 33 ^ uint64_t(scissor.extent.width); - hash = hash * 33 ^ uint64_t(scissor.extent.height); - } + uint64_t hash = uint64_t(vk_fragment_module); + hash = hash * 33 ^ XXH3_64bits(viewports.data(), viewports.size() * sizeof(VkViewport)); + hash = hash * 33 ^ XXH3_64bits(scissors.data(), scissors.size() * sizeof(VkRect2D)); + return hash; } }; @@ -152,7 +153,25 @@ struct VKGraphicsInfo { bool operator==(const FragmentOut &other) const { +#if 1 return hash() == other.hash(); +#else + if (depth_attachment_format != other.depth_attachment_format || + stencil_attachment_format != other.stencil_attachment_format || + vk_render_pass != other.vk_render_pass || + color_attachment_formats.size() != other.color_attachment_formats.size()) + { + return false; + } + + if (memcmp(color_attachment_formats.data(), + other.color_attachment_formats.data(), + color_attachment_formats.size() * sizeof(VkFormat)) == 0) + { + return false; + } + return true; +#endif } uint64_t hash() const @@ -160,10 +179,8 @@ struct VKGraphicsInfo { uint64_t hash = uint64_t(vk_render_pass); hash = hash * 33 ^ uint64_t(depth_attachment_format); hash = hash * 33 ^ uint64_t(stencil_attachment_format); - for (VkFormat color_attachment_format : color_attachment_formats) { - hash = hash * 33 ^ uint64_t(color_attachment_format); - } - + hash = hash * 33 ^ XXH3_64bits(color_attachment_formats.data(), + color_attachment_formats.size() * sizeof(VkFormat)); return hash; } };