search

KhronosGroup / SPIRV-Tools

Apache License 2.0
1.09k stars 559 forks source link

FriendlyNameMapper::to_string(uint32_t) blocking on linux #5802

Closed jeroenbakker-atmind closed 2 months ago

jeroenbakker-atmind commented 2 months ago

Hi!

Blender developer working on integrating Vulkan backend into Blender. Shader compilation performance is really important in our usecase as precompilation is not really possible. However when running more optimizers in parallel the performance degrades very fast due to stdlib implementation.

See the next hotspot where we compile hundreds of shaders using all threads available on the system. It takes almost as long as running on a single thread.

image

Via hotspot the issue tracks down as stdlib that uses global lock when accessing the locale. This could be evaded by changing the to_string(uint32_t) implementation at https://github.com/KhronosGroup/SPIRV-Tools/blob/main/source/name_mapper.cpp#L34 This part of the code doesn't require any locale but as noted in the comments is done this way for certain android compilers/platforms.

Would it be possible to reevaluate the current implementation so it would not stall when using multithreaded compilation on Linux?

I am running on a regular Ubuntu 24.04 Linux-6.8.0-41-generic-x86_64-with-glibc2.39 64 Bits, X11 UI.

jeroenbakker-atmind commented 2 months ago

We are experimenting with using C17 std::to_chars See https://projects.blender.org/blender/blender/pulls/127418/files Perhaps using this when CPP17 is defined it would be an option as many parts of the std uses this global lock.

arcady-lunarg commented 2 months ago

Doesn't SPIRV-Tools already require C++17? https://github.com/KhronosGroup/SPIRV-Tools/blob/7c9210cc1d82e8bf0ca969a88db069c83d838398/CMakeLists.txt#L38 The comment about std::to_string was added in 2016, and may not be relevant anymore.

s-perron commented 2 months ago

I'm discussing this internally. When we started to require C++17 for spirv-tools, we broke the compilers that this workaround was trying to help. I don't think we have the requirement anymore. I'll make the change.

Hugobros3 commented 2 months ago

Do you need human-readable names for the instructions ? If not the CLI --raw-id and corresponding SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES options should skip the string operations entirely.