Fix RID_Owner synchronization

[RandomShaper]

Includes #85167, because this PR needs SpinLock and extends it with non-locking acquire and release methods.

Now that RID_Owner is lockless for querying, some additional measures needed to be taken to avoid data races and ensure proper memory ordering.

NOTE: A subset of the issues that arise from the lack of proper synchronization would only be apparent in architectures with a purely weakly ordered memory model. Even ARM is able to dampen them somehow due to its handling of data dependencies. However, the code should account for those cases so it's valid despite the memory model. By the way, architectures with such a memory model aren't purely theoretical (cough, #97822, cough).

In short, this PR does the following:

• Adds the proper usage of atomics and memory ordering constraints so RID_Owner is correct for the current known usages.
• Extends SpinLock with acquire() and release() methods that allow to synchronize to the data normally protected by it in a lockless fashion. Also, the change from SpinLock to Mutex in RID_Alloc is reverted, naturally. The new SpinLock implementation is nicer on contention, which makes it somewhat closer to Mutex in that regard.
• Implements some test cases that would cause sanitizer errors (true positives; there would also be some false positives, but they are avoided by telling tsan we know what we are doing).

2024-11-04:
I've decided toi make this simpler by abandoning the idea of having a SpinLock that also supports lockless access. So, I've updated the code with some markers and remarks in comments that should be good enough for now at least.

[darksylinc]

I'm not fully sure what is being solved here but a few things:

1. Spin locks block too. The difference with a Mutex is whether they burn CPU cycles while blocked, and make it harder to detect deadlocks (a spinlock deadlocking is called a livelock and are harder to detect because the system appears to be busy doing useful stuff)

2. A good mutex implementation already has (should) acquire/release semantics. Thus you shouldn't have to worry about relaxed memory ordering if the code is protected by lock/unlock. std::mutex should already have them.

3. Mutex implementations come in many flavors (this is important and I want to explain it in detail for everyone, because you rarely see people talk about this):

a. Mutex vs Futex. Historically speaking, a mutex is a sync primitive that when you lock() it enters kernel space so the OS blocks the thread if needed, and when you unlock() it also enters kernel space. This kernel space transition is often overkill for frequently called code. Specially nowadays with Spectre mitigations.

That's where futexes enter. You can think of a Futex as a Mutex wrapped by a Spinlock that spins once (or a couple times). If the futex sees it should keep spinning, then it enters the mutex to go to sleep.

The main issue is that "mutex" is loosely defined, so some platforms implement their mutexes as futexes, while others implement mutexes as OS primitives.

b. FIFO vs LIFO vs Random. Sometimes referred to as a "fair" mutex. Another issue with Mutex is whether they have FIFO behavior or not. Of course "FIFO" is loose here because two threads may lock at "exactly" the same time, and time is relative (where one sees A and B happened at the same time, another one may see B came before A, while another sees A came after B). That's why calling them "fair" is more accurate. Consider the following:

• Thread A enters locks().
• Thread B blocks on locks(). Wants to do a light work on a variable.
• Thread C blocks on locks(). Wants to iterate extremely frequently.

An unfair mutex may end up in the following unwanted scenario:

• Thread A leaves the unlock().
• Thread C enters lock(). Leaves unlock(). Enters again. And repeat.
• Thread B gets blocked for potentially forever.

A FIFO/fair mutex does extra work to ensure B gets a chance to run. It may not be able to guarantee B enters the block before C, but it guarantees C doesn't steal B's chance to run forever.

Godot seems to be using std::mutex, which is not guaranteed to be fair. Thus Godot's behavior is at the whims of the std implementation.

By switching to a spinlock, you ensure Godot's behavior is consistent between OSes, but now you're at the whims of the HW, and HW changes their behavior.

My point is that this change you're witnessing in contention is more likely the change of futex vs mutex, or the change in fairness behavior, rather than a result of using a spinlock algorithm.

Unfortunately we programmers say "use a mutex", but those experienced enough and burnt by past experiences will ask "which mutex"?

1. A fair futex? (hard to achieve)
2. An unfair futex?
3. An unfair mutex?
4. A fair mutex?

Stack Overflow has an answer of 3 different mutex implementations available on Windows: CRITICAL_SECTION (futex, very unfair), Mutex objects (mutex, fair), and SRW (futex, unfair, but more fair than CS).

So what now?

There are various things that can be tried:

1. Instead of using std::mutex, on Windows try rolling own implementations with all 3 (CS, Mutex, SRW locks) and test their behaviors.
2. Rolling our own futex is also possible. OgreNext has its own for Windows (header).

[RandomShaper]

All that is interesting and we may really want to reconsider the overall approach to mutexing in Godot.

However, what this PR is solving is in the context of the current state of affairs.

---

My point is that this change you're witnessing in contention is more likely the change of futex vs mutex, or the change in fairness behavior, rather than a result of using a spinlock algorithm.

The benchmark is in the PR about modifying the SpinLock, so it's former SpinLock vs. revised SpinLock.

---

2. A good mutex implementation already has (should) acquire/release semantics. Thus you shouldn't have to worry about relaxed memory ordering if the code is protected by lock/unlock. std::mutex should already have them.

What we need in this PR is something like a mutex that, besides providing acq-rel critical sections, also provide acq-rel for fragments of code that will run without locking (the user of such a sync primitive must ensure the data won't be further modified, which is the case here in RID_Alloc::get_or_null()).

[RandomShaper]

Trying to be pragmatic, I've made a simplification. Please see updated description.

[clayjohn]

CC @darksylinc What do you think about the changes?

[darksylinc]

On line 124 Pedro wrote:

class Thread {
public:
	typedef void (*Callback)(void *p_userdata);

	typedef uint64_t ID;

	static constexpr size_t CACHE_LINE_BYTES = sizeof(void *);

Is this a typo? It is overriding CACHE_LINE_BYTES with a different value for this scope (Threads).

If it is intended, it should have comments on why you're doing this (since 4 or 8 bytes is definitely not the size of a cache line).

[darksylinc]

This is not enough on systems with strong memory models.

Not because of HW reasons, but compiler ones. x86 will not reorder your memory operations, but compiler optimizations still can.

You still need std::atomic_thread_fence() to tell the compiler not to reorder stuff in ways that would break your code.

[RandomShaper]

Since the acquire and release points are at the beginning and end of functions, I was just relying on functions calls acting as compiler barriers. That said, I don't think some extra safety will cause any harm. I've updated the code. However, I'm adding compiler barriers there (std::atomic_signal_fence()) instead.

[RandomShaper]

On line 124 Pedro wrote:

class Thread {
public:
	typedef void (*Callback)(void *p_userdata);

	typedef uint64_t ID;

	static constexpr size_t CACHE_LINE_BYTES = sizeof(void *);

Is this a typo? It is overriding CACHE_LINE_BYTES with a different value for this scope (Threads).

If it is intended, it should have comments on why you're doing this (since 4 or 8 bytes is definitely not the size of a cache line).

Note that's in the #if case for threadless builds. In any case, this commit was already merged as part of #85167. I'll rebase this PR and it will fade away from here.

[hpvb]

Had a call with @RandomShaper and we went through this in excruciating detail. I'm convinced this is correct and desirable.

[RandomShaper]

I just realized I have to make a few thread safety measures only happen for the thread safe case. Not the end of the world otherwise, but let's aim for 100% accuracy. I'll force-push shortly.

[RandomShaper]

Done.

[akien-mga]

Thanks!