Fix bugs with remote entity allocation

[ElliottjPierce]

Objective

Fixes #24897.

I thought I could be clever here, but @Imberflur, who apparently reads random atomics code in their free time (which is awesome,) discovered some UB issues in the new entity allocator from #18670. I believe I've fixed those issues in this PR, but I also believed there weren't any to begin with, so that's not exactly authoritative. Now, the last time this code was reviewed, 11 people reviewed it (12 including myself) and none of use saw this, so while this should truly fix everything, it definitely deserves some eagle-eyed reviewing!

In the future, we could consider doing some form of loom or shuttle testing here, but that's not for this PR, and there are good reasons to not do this too. (loom can be extraordinarily slow for tests and shuttle is non-deterministic by nature, IIRC.)

Solution

See #24897 for the details on the initial bugs, but we basically need to use relaxed atomics for slots and add acquire/release ordering for how remote_alloc interacts with free.

Additionally, I made the Slot type have identical memory layout and bit validity as Entity. That means non-remote alloc can skip the atomics because it already has strict ordering with free.

The only really tricky part is handling Slot on platforms that don't have 64-bit atomics. We could do nothing and let portable atomics do the work, but that could be a tad slow on some platforms because we don't need true atomics. That is, we don't care if the u64 we get back is "whole and correct" or not; we just need it to not race. I've opted to split it into 32-bit atomics on those platforms. That adds significant complexity though, so if we can accept slightly worse performance on those platforms, it would be nice to just use AtomicU64 everywhere. IDK what the real-world performance difference would be, but I don't have a way to benchmark it either.

Testing

All tests pass, etc, but they did last time, too, so that doesn't mean much with these fixes. It might also be nice to test this on a platform without 64-bit atomics, but I can't do that locally and don't know if CI does it either.

I have not benchmarked this for two reasons. First, on most platforms, like x86, this should be functionally the same as it was before, and second, I don't think it can be any faster without re-introducing UB or a new allocator paradigm (and I don't have a v10 hidden up my sleeves).

[chescock]

Why have a special case here? It adds a lot of complexity, and a non-atomic load isn't any faster than a Relaxed load.

If we do need a special case here, then I'd prefer to avoid relying on the implementation details of Entity. This could be cast::<u64> followed by from_bits, which should compile down to the same thing, but would only rely on the layout of Slot and not the layout and interpretation of Entity and all of the wrapper types it's built on.

(This would also remove the need for #[repr(align(8))].)

[ElliottjPierce]

For now, I've kept it and changed this to use from_bits. That's a great idea.

Why have a special case here? It adds a lot of complexity, and a non-atomic load isn't any faster than a Relaxed load.

Agreed on the complexity, but I mean, it's not so bad, especially if it's ok to remove the 32-bit atomics shenanigans. Plus, non-atomic ops could be faster here in some cases. I'm no asm expert, but it might be possible that FreeBufferIterator could see some optimizations from not needing any atomics.

In short, I didn't do this because I thought it would improve performance. I did it because I didn't know that not doing it wouldn't hurt performance. But if anyone feels strongly about this, I suppose it can be removed. It certainly doesn't affect x86. IDK about arm.

[chescock]

In short, I didn't do this because I thought it would improve performance. I did it because I didn't know that not doing it wouldn't hurt performance. But if anyone feels strongly about this, I suppose it can be removed. It certainly doesn't affect x86. IDK about arm.

I won't block on it, but I would recommend removing it. This is tricky unsafe code, so we should try to make it as clear and readable as possible, and only add complexity when necessary for performance. Having a const generic parameter and several extra safety requirements to check is quite a bit of complexity for something that we know won't affect x86 and have no reason to believe will affect other architectures.