Eevee: Race condition in ShaderCreateInfo::finalize() #128164

New Issue

Sergey Sharybin · 2024-09-26T10:08:40+02:00

Sergey Sharybin commented

2024-09-26 10:08:40 +02:00

System Information
Operating system: macOS-14.6.1-arm64-arm-64bit 64 Bits
Graphics card: Apple M2 Ultra Apple 4.1 Metal - 88.1

Blender Version
Broken: Recent main (8dcd51ddc3)

Short description of error
It appears that ShaderCreateInfo::finalize can be called from multiple threads for the same ShaderCreateInfo, resulting in the wrong shader: the second thread will return early since the system believes the finalization is done.

This seems to be the root cause of Eevee benchmarks failing on the buildbot: https://builder.blender.org/admin/#/builders/243/builds/223

The error there is coming from the shader compilation error: # error Missing draw_view additional create info on shader create info. This appears to be due to incomplete shader info used to compile some shaders.

Exact steps for others to reproduce the error

For the simplicity of reproducing the error apply the patch attached
Open tests/benchmarks/eevee/reports/T98989.blend
Observe: ShaderCreateInfo::finalize name:eevee_surf_deferred called while other thread is finalizing it

P.S. This is more of a write-up and coordination with Clément (and the PoC code to help triggering the issue) after a pair debugging session.

**System Information** Operating system: macOS-14.6.1-arm64-arm-64bit 64 Bits Graphics card: Apple M2 Ultra Apple 4.1 Metal - 88.1 **Blender Version** Broken: Recent main (8dcd51ddc3e24fed803aed06f9e63dfeee821545) **Short description of error** It appears that `ShaderCreateInfo::finalize` can be called from multiple threads for the same `ShaderCreateInfo`, resulting in the wrong shader: the second thread will return early since the system believes the finalization is done. This seems to be the root cause of Eevee benchmarks failing on the buildbot: https://builder.blender.org/admin/#/builders/243/builds/223 The error there is coming from the shader compilation error: `# error Missing draw_view additional create info on shader create info`. This appears to be due to incomplete shader info used to compile some shaders. **Exact steps for others to reproduce the error** - For the simplicity of reproducing the error apply the patch attached - Open `tests/benchmarks/eevee/reports/T98989.blend` - Observe: `ShaderCreateInfo::finalize name:eevee_surf_deferred called while other thread is finalizing it` P.S. This is more of a write-up and coordination with Clément (and the PoC code to help triggering the issue) after a pair debugging session.

finalize.patch

1.9 KiB

Sergey Sharybin added the

labels 2024-09-26 10:09:01 +02:00

Clément Foucault self-assigned this 2024-09-26 12:08:27 +02:00

Jesse Yurkovich added the

Module

Viewport & EEVEE

label 2024-09-27 08:49:59 +02:00

Clément Foucault removed their assignment 2024-09-27 17:25:26 +02:00

Clément Foucault commented

2024-09-27 17:25:47 +02:00

Leaving @pragma37 to take a look

👍 1

Miguel Pozo self-assigned this 2024-09-27 17:26:56 +02:00

Miguel Pozo commented

2024-09-27 19:33:42 +02:00

I assumed this would have been caused by the recent draw_manager_shader refactors, but doesn't seem to be the case.

The issue here seems that we are compiling, at the same time, deferred and non-deferred shaders (so, different threads) with the same additonal_info dependencies.
(No idea why this didn't surface earlier)

So, we either ensure that all additional_info dependencies are finalized from the main thread (maybe we can finalize directly at the additional_info call ?), or add mutexes to every ShaderCreateInfo (and ensure there are no cyclic dependencies).

~~Using mutexes seems to do the trick, although I'm running into weird issues because the ShaderCreateInfo constructor doesn't seem to be called in all cases.~~
Nope, it was lack of copy constructor.

I assumed this would have been caused by the recent `draw_manager_shader` refactors, but doesn't seem to be the case. The issue here seems that we are compiling, at the same time, deferred and non-deferred shaders (so, different threads) with the same `additonal_info` dependencies. (No idea why this didn't surface earlier) So, we either ensure that all `additional_info` dependencies are finalized from the main thread (maybe we can finalize directly at the `additional_info` call ?), or add mutexes to every `ShaderCreateInfo` (and ensure there are no cyclic dependencies). ~~Using mutexes seems to do the trick, although I'm running into weird issues because the `ShaderCreateInfo` constructor doesn't seem to be called in all cases.~~ Nope, it was lack of copy constructor.

👍 1 🚀 1

Miguel Pozo referenced a pull request that will close this issue

2024-09-27 20:25:00 +02:00

Fix #128164: Race condition in ShaderCreateInfo::finalize() #128281

Sign in to join this conversation.