Batch of fixes for WorkerThreadPool and ResourceLoader

[RandomShaper]

While doing some heavy testing with projects that really stress resource loading, I've found a number of edge cases and regressions that required attention. I've squashed them in as few commits as it made sense and I'm submitting them all in a single PR.

I'll keep testing this with more projects before taking it out ot draft. 2024-07-18: After some tweaks and additional testing, verifying a 100% deadlock-free loading experience on my end with complex projects and no apparent regressions, I'm taking this out of draft.

2024-07-19: Now this is based on the extracted safe subset in #94526, I'm removing the descriptions of the commits moved there. Keeping the commits themselves to avoid solving conflicts that won't arise once the other PR has been merged.

---

ResourceLoader: Properly push & pop TLS state on recursive load tasks

---

ResourceLoader: Enhance deadlock prevention

Benefits:

• Simpler code. The main load function is renamed so it's apparent that it's not just a thread entry point anymore.
• Cache and thread modes of the original task are honored. A beautiful consequence of this is that, unlike formerly, re-issued loads can use the resource cache, which makes this mechanism much more performant.
• The newly added getter for caller task id in WorkerThreadPool allows to remove the custom tracking of that in ResourceLoader.
• The check to replace a cached resource and the replacement itself happen atomically. That fixes deadlock prevention leading to multiple resource instances of the same one on disk. As
  a side effect, it also makes the regular check for replace load mode more robust.

---

ResourceLoader: Optimize remap check by deferring until a non-mutex zone

---

ResourceLoader: Fix edge cases in the management of user tokens

1. Make handling of user tokens atomic:
   Loads started with the external-facing API used to perform a two-step setup of the user token. Between both, the mutex was unlocked without its reference count having been increased. A non-user-initiated load could therefore destroy the load task when it unreferenced the token.
   Those stages now happen atomically so in the one hand, the described race condition can't happen so the load task life insurance doesn't have a gap anymore and, on the other hand, the ugliness that the call to load could return ERR_BUSY if happening while other thread was between both steps is gone.
   The code has been refactored so the user token concerns are still outside the inner load start function, which is agnostic to that for a cleaner implementation.
2. Clear ambiguity between load operations running on WorkerThreadPool:
   The two cases are: single-loaded thread directly started by a user pool task and a load started by the system as part of a multi-threaded load.
   Since ensuring all the code dealing with this distinction would make it very complex, and error-prone, a different measure is applied instead: just take one of the cases out of the dichotomy. We now ensure every load happening on a pool thread has been initiated by the system.
   The way of achieving that is that a single-threaded user-started load initiated from a pool thread, is run as another task.

---

WorkerThreadPool (plus friends): Overhaul unlock allowance zones

This fixes a rare but possible deadlock, maybe due to undefined behavior. The new implementation is safer, at the cost of some added boilerplate.

---

Fixes #95616.

[clayjohn]

Given the complexity of this PR, I think we should probably aim to merge it for 4.4 and then cherry pick to 4.3.1

[akien-mga]

Would be good to fix the CI errors, it seems to have warnings on all compilers.

[RandomShaper]

Would be good to fix the CI errors, it seems to have warnings on all compilers.

I'm trying. I love the boilerplate and weird syntax involved in C++ templates. 😅

[RandomShaper]

I've given up on the warning about certain template thing. It's a Clang-only thing and, in any case, if a template instantiation is really missing, linking will fail later anyway. So, I've added -Wno-undefined-var-template for warnings=extra on Clang.

[akien-mga]

So this is a lot of changes to core days before RC 1, which I'm worried about. These areas are prone to regressions and we don't have time for a lot of back and forth with multiple RCs to fix them (and stress to solve regressions in between each RC build).

How confident are you in those changes?

Is there a subset of those we can merge for 4.3, especially to fix the regression(s) from previous PRs, and some that can be kept for 4.4 / 4.3.1 after the 4.3-stable release?

[RandomShaper]

Conflicts solved.

[akien-mga]

Thanks!

[aaronfranke]

Godot doesn't compile on my system as of this PR (macOS 14.6.1, Clang 15). The merge commit before 04f6321 compiles, but the merge commit of this PR 0ba0be2 fails to compile with this error:

[ 18%] Compiling modules/gdscript/gdscript.cpp ...
In file included from modules/gdscript/gdscript.cpp:31:
In file included from modules/gdscript/gdscript.h:39:
In file included from ./core/io/resource_loader.h:36:
In file included from ./core/object/worker_thread_pool.h:34:
In file included from ./core/os/condition_variable.h:35:
./core/os/safe_binary_mutex.h:67:9: error: instantiation of variable 'SafeBinaryMutex<2>::tls_data' required here, but no definition is available [-Werror,-Wundefined-var-template]
                if (++tls_data.count == 1) {
                      ^
./core/os/safe_binary_mutex.h:100:9: note: in instantiation of member function 'SafeBinaryMutex<2>::lock' requested here
                mutex.lock();
                      ^
modules/gdscript/gdscript.cpp:772:15: note: in instantiation of member function 'MutexLock<SafeBinaryMutex<2>>::MutexLock' requested here
                                MutexLock lock(GDScriptCache::singleton->mutex);
                                          ^
./core/os/safe_binary_mutex.h:63:30: note: forward declaration of template entity is here
        static thread_local TLSData tls_data;
                                    ^
./core/os/safe_binary_mutex.h:67:9: note: add an explicit instantiation declaration to suppress this warning if 'SafeBinaryMutex<2>::tls_data' is explicitly instantiated in another translation unit
                if (++tls_data.count == 1) {
                      ^
1 error generated.
scons: *** [modules/gdscript/gdscript.macos.editor.arm64.o] Error 1
scons: building terminated because of errors.

[RandomShaper]

Cherry-pick for 4.3 included in #96606.