Use separate WorkThreadPool for shader compiler. #103506
Conversation
akien-mga
added
the
cherrypick:4.4
| WorkerThreadPool *WorkerThreadPool::get_named_pool(const StringName &p_name) { | ||
| WorkerThreadPool **pool_ptr = named_pools.getptr(p_name); | ||
| if (pool_ptr) { | ||
| return *pool_ptr; | ||
| } else { | ||
| WorkerThreadPool *pool = memnew(WorkerThreadPool(false)); | ||
| pool->init(); | ||
| named_pools[p_name] = pool; | ||
| return pool; | ||
| } | ||
| } |
There was a problem hiding this comment.
I would advocate for a separate API to create a named pool and have this API fail if the pool doesn't exist.
There was a problem hiding this comment.
If it was a public API and intended for generic use, I would agree. But it's only intended for a single case only, and in general should be avoided.
|
I'm a fan of the concept of named pools, but does this implementation mean that it'll double up the available thread count from whatever the user configured? I've looked at the PR and didn't see if we have any logic to handle on how to respect that setting. It's still an acceptable hotfix in my eyes as the workload for shader compilation doesn't come in that frequently. |
There was a problem hiding this comment.
Overall I think this does the job considering this is a hotfix for a critical issue that has presented itself with 4.4, but I think we should consider extending WorkerThreadPool to work with a design where we're just allowed to create pools locally to the context of what we want to use. This is not entirely necessary for this PR given the nature of the issue at the moment.
For example, instead of a name, we'd just make the object inside ShaderRD's global context instead, which would make it a bit more natural and it could allow us to specify a thread count specifically for shader compilation easily.
Yes, it is doubling the number of threads (we do not want to constantly spawn new threads, but simply having 2 threads per core should not cuase any problems). |
|
I am a little bit unclear about the tradeoffs here. We are spawning a bunch of extra threads that will ultimately sit idle. Will they consume any resources once we finish compiling shaders? Are their any OS limitations we need to be wary of (especially on Mobile/Web)? |
We do not have WebGPU support (RenderingDevice), so it will have zero effect on web.
There is a per-process handle limit, but as far as I know it's in the thousands or tens of thousands depending on platform.
512 KiB of memory for stack + metadata per thread. At least on macOS, Xcode profiler show completely flat line for idle threads with no detectable CPU activity at all. Also, the new pool is only initialized if there are shaders to compile, as long as it's loaded from cache it won't, so it should affect only the first start or editor. |
That's the main reason I do not want to create a nice API for it, and implemented it as a single function. It won't prevent it from being used, but at least have more chances for a comment advising not to use it to be noticed. |
|
Thanks! |
|
Cherry-picked for 4.4.1. |
akien-mga
removed
the
cherrypick:4.4
Use separate WorkThreadPool for shader compiler.
Fixes #102812
Supersede #103299
Creates dedicated thread pool for
ShaderRDcompilation.Not a perfect solution, but seems to be working pretty reliably (I was not able to get it crash or deadlock a single time).