[ZH] Unify code of AsciiString

[Mauller]

• Merge after: [GEN][ZH] Add endian compat for BIGFileSystems #798

This PR unifies the implementation of AsciiString between Generals and Zero Hour by using the Generals version of AsciiString in Zero Hour.

Zero Hour had to use a different scoped mutex implementation as they inlined a few of the string access functions. They likely thought this would help performance but it lead to a messier implementation.

The Generals version of AsciiString is also more cross compatible than the Zero Hour version as it is not dependend on windows functions. And the scoped section can have its implementation altered independent of code that makes use of it.

[xezon]

This change needs to be tested for compatibility. The inlining and critical section change may affect code generation at scale.

[xezon]

This cast is bad, but I do agree with the atomic increment here. We do not want a critical section here. Not yet sure what the clean way for a 2 byte increment here is. Unfortunately InterlockedIncrement16 only comes with Windows 8.

We can make a follow up change and get rid of the Critical Sections.

For interlocked compat, we can start with:
https://github.com/xezon/Thyme/blob/8f26fdb00adf17aa85a00e509db6a2a4b8c2da9a/src/game/common/utility/atomicop.h

But we need a plan for 2 byte atomic op.

Perhaps we can do it with assembler: lock inc word [mem]

[Mauller]

The other horrid thing is that it seems that all instances of Ascii string share the same mutex.

Or that is how i interpreted it based on what is in the critical section files.

[OmniBlade]

Ideally we would use std::atomic would we not? Maybe just throw some ifdef around the Interlocked* calls and the refcount variable? Yeah the critical section is shared so operations that lock on any string affect all strings.

[xezon]

Perhaps do

#if VS6

#define ATOMIC_16 volatile AtomicType16
#define ATOMIC_32 volatile AtomicType32
#define ATOMIC_64 volatile AtomicType64

inline AtomicType16 InterlockedIncrement(volatile AtomicType16*)
{
  ...
}
inline AtomicType32 InterlockedIncrement(volatile AtomicType32*)
{
  ...
}
...

#else

#define ATOMIC_16 std::atomic<AtomicType16>
#define ATOMIC_32 std::atomic<AtomicType32>
#define ATOMIC_64 std::atomic<AtomicType64>

inline AtomicType16 InterlockedIncrement(std::atomic<AtomicType16>*)
{
  ...
}
inline AtomicType32 InterlockedIncrement(std::atomic<AtomicType32>*)
{
  ...
}
...

#endif

[Mauller]

This change needs to be tested for compatibility. The inlining and critical section change may affect code generation at scale.

Yeah i was thinking that, the zero hour version just looks like a really horrid case of trying to improve performance.

[Mauller]

Any further thoughts on this? i think the critical section changes could come with a future change to look over critical sections use across the codebase.

[xezon]

Yes, is there a plan for follow up or not?

[Mauller]

Yes, is there a plan for follow up or not?

I didn't have one initially, but i could look at the critical sections and bringing across an atomic compat similar to what thyme has if we think it is a sensible start.

I will look at getting another PR together that builds off this PR then both could be dealt with in sync.

[Mauller]

Rebased with recent Main for continued work on dependend PR's

[xezon]

Was this ran against VC6 Golden Replay?

[Mauller]

Was this ran against VC6 Golden Replay?

I think i ran it, can do it quickly to double check.

[Mauller]

Tested with GR1 and it ran fine with no mismatch. Actually ran really well tbh without any hitches or slowdowns when spedup.

[xezon]

After this change we can try move the String files to Core. And then afterwards make the reference counter atomic and get rid of the Critical Sections.

[Mauller]

After this change we can try move the String files to Core. And then afterwards make the reference counter atomic and get rid of the Critical Sections.

Just getting a draft together for an interlocked_compat for that.