Skip to content

perf(String): Remove global locks on reference counters and replace them with atomic variables in string classes #3495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Mauller wants to merge 2 commits into
base: main
Choose a base branch
from
+37 −61
Open

perf(String): Remove global locks on reference counters and replace them with atomic variables in string classes #3495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e78336
perf(AsciiString): Remove global lock on reference count and replace …
Mauller Oct 12, 2026
1ab0c84
perf(UnicodeString): Remove global lock on reference count and replac…
Mauller Oct 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions Benchmarks/Google/Core/GoogleBenchmarkMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,17 @@ class EngineCriticalSections
public:
EngineCriticalSections()
{
TheAsciiStringCriticalSection = &m_asciiString;
TheUnicodeStringCriticalSection = &m_unicodeString;
TheDmaCriticalSection = &m_dma;
TheMemoryPoolCriticalSection = &m_memoryPool;
}

~EngineCriticalSections()
{
TheAsciiStringCriticalSection = nullptr;
TheUnicodeStringCriticalSection = nullptr;
TheDmaCriticalSection = nullptr;
TheMemoryPoolCriticalSection = nullptr;
}

private:
CriticalSection m_asciiString;
CriticalSection m_unicodeString;
CriticalSection m_dma;
CriticalSection m_memoryPool;
};
Expand Down
7 changes: 4 additions & 3 deletions Core/GameEngine/Include/Common/AsciiString.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#include "Lib/BaseType.h"
#include "Common/Debug.h"
#include "Common/Errors.h"
#include <Utility/atomic_adapter.h>

class UnicodeString;

Expand Down Expand Up @@ -80,11 +81,11 @@ class AsciiString
{
private:

// Note, this is a Plain Old Data Structure... don't
// add a ctor/dtor, 'cuz they won't ever be called.
// This header data is allocated in raw storage with an explicit lifetime.
// It is constructed when string data is allocated and destroyed when the string is freed.
struct AsciiStringData
{
unsigned short m_refCount; // reference count
std::atomic<unsigned short> m_refCount; // reference count
unsigned short m_numCharsAllocated; // length of data allocated
// char m_stringdata[];

Expand Down
7 changes: 4 additions & 3 deletions Core/GameEngine/Include/Common/UnicodeString.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#include "Lib/BaseType.h"
#include "Common/Debug.h"
#include "Common/Errors.h"
#include <Utility/atomic_adapter.h>

class AsciiString;

Expand Down Expand Up @@ -80,11 +81,11 @@ class UnicodeString
{
private:

// Note, this is a Plain Old Data Structure... don't
// add a ctor/dtor, 'cuz they won't ever be called.
// This header data is allocated in raw storage with an explicit lifetime.
// It is constructed when string data is allocated and destroyed when the string is freed.
struct UnicodeStringData
{
unsigned short m_refCount; // reference count
std::atomic<unsigned short> m_refCount; // reference count
unsigned short m_numCharsAllocated; // length of data allocated
// WideChar m_stringdata[];

Expand Down
26 changes: 13 additions & 13 deletions Core/GameEngine/Source/Common/System/AsciiString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
///////////////////////////////////////////////////////////////////////////////
#include "PreRTS.h" // This must go first in EVERY cpp file in the GameEngine

#include "Common/CriticalSection.h"
#include "WWLib/utf8.h"


Expand Down Expand Up @@ -121,7 +120,6 @@ static StringCaseInfo getStringCaseInfo(const char *str)
// -----------------------------------------------------
AsciiString::AsciiString(const AsciiString& stringSrc) : m_data(stringSrc.m_data)
{
ScopedCriticalSection scopedCriticalSection(TheAsciiStringCriticalSection);
if (m_data)
++m_data->m_refCount;
validate();
Expand Down Expand Up @@ -184,7 +182,8 @@ void AsciiString::ensureUniqueBufferOfSize(int numCharsNeeded, Bool preserveData
DEBUG_ASSERTCRASH(numCharsNeeded <= MAX_LEN, ("AsciiString::ensureUniqueBufferOfSize exceeds max string length %d with requested length %d", MAX_LEN, numCharsNeeded));
int minBytes = sizeof(AsciiStringData) + numCharsNeeded*sizeof(char);
int actualBytes = TheDynamicMemoryAllocator->getActualAllocationSize(minBytes);
AsciiStringData* newData = (AsciiStringData*)TheDynamicMemoryAllocator->allocateBytesDoNotZero(actualBytes, "STR_AsciiString::ensureUniqueBufferOfSize");
void* rawData = TheDynamicMemoryAllocator->allocateBytesDoNotZero(actualBytes, "STR_AsciiString::ensureUniqueBufferOfSize");
AsciiStringData* newData = new(rawData) AsciiStringData;
newData->m_refCount = 1;
newData->m_numCharsAllocated = (actualBytes - sizeof(AsciiStringData))/sizeof(char);

Expand Down Expand Up @@ -214,16 +213,17 @@ void AsciiString::ensureUniqueBufferOfSize(int numCharsNeeded, Bool preserveData
// -----------------------------------------------------
void AsciiString::releaseBuffer()
{
ScopedCriticalSection scopedCriticalSection(TheAsciiStringCriticalSection);

validate();
if (m_data)
{
if (--m_data->m_refCount == 0)
AsciiStringData* oldData = m_data;
m_data = nullptr;

if (--oldData->m_refCount == 0)
{
TheDynamicMemoryAllocator->freeBytes(m_data);
oldData->~AsciiStringData();
TheDynamicMemoryAllocator->freeBytes(oldData);
}
m_data = nullptr;
}
validate();
}
Expand Down Expand Up @@ -253,15 +253,15 @@ AsciiString::AsciiString(const char* s, int len) : m_data(nullptr)
// -----------------------------------------------------
void AsciiString::set(const AsciiString& stringSrc)
{
ScopedCriticalSection scopedCriticalSection(TheAsciiStringCriticalSection);

validate();

@macroscopeapp macroscopeapp Bot Oct 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High System/AsciiString.cpp:252

Concurrent assignment can increment the reference count of a buffer after another thread has freed it, causing a use-after-free and corrupting ownership. set reads stringSrc.m_data and increments its reference count without the critical-section guard; restore the guard around this ownership handoff.

Suggested change
validate();
ScopedCriticalSection scopedCriticalSection(TheAsciiStringCriticalSection);
validate();
Also found in 1 other location(s)

Core/GameEngine/Source/Common/System/UnicodeString.cpp:69

Removing the lock from UnicodeString copy construction leaves copy-on-write mutation unsynchronized. ensureUniqueBufferOfSize() checks m_refCount == 1 and then writes into the shared buffer; another thread can execute this constructor between that check and the write and increment the atomic count, so the mutating thread modifies storage now owned by both strings. Concurrent readers/writers can therefore observe corrupted string contents. Making only the counter atomic does not make the check-then-mutate sequence safe.

🤖 Copy this AI Prompt to have your agent fix this: 
In file @Core/GameEngine/Source/Common/System/AsciiString.cpp around line 252:

Concurrent assignment can increment the reference count of a buffer after another thread has freed it, causing a use-after-free and corrupting ownership. `set` reads `stringSrc.m_data` and increments its reference count without the critical-section guard; restore the guard around this ownership handoff.

Also found in 1 other location(s):
- Core/GameEngine/Source/Common/System/UnicodeString.cpp:69 -- Removing the lock from `UnicodeString` copy construction leaves copy-on-write mutation unsynchronized. `ensureUniqueBufferOfSize()` checks `m_refCount == 1` and then writes into the shared buffer; another thread can execute this constructor between that check and the write and increment the atomic count, so the mutating thread modifies storage now owned by both strings. Concurrent readers/writers can therefore observe corrupted string contents. Making only the counter atomic does not make the check-then-mutate sequence safe.

@Mauller Mauller Oct 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@macroscopeapp macroscopeapp Bot Oct 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This remains unprotected: set increments stringSrc.m_data's refcount without synchronization, so concurrent destruction/assignment of stringSrc can still free it first.

@Mauller Mauller Oct 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a local mutex on the object work?

@macroscopeapp macroscopeapp Bot Oct 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not if it locks only the destination. The handoff must hold a lock on stringSrc while reading m_data and incrementing its refcount; locking both source and destination needs a consistent address-based order to avoid a.set(b)/b.set(a) deadlocks.

@macroscopeapp macroscopeapp Bot Oct 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes—if every ownership acquire and release uses it. It must cover source m_data read + increment, and decrement + conditional free (including copy construction). That restores class-wide serialization, effectively the removed global lock.

@Mauller Mauller Oct 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have altered the implementation now, we now move the resources to local varaibles before handling the reference counters.

@macroscopeapp macroscopeapp Bot Oct 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still races: copying stringSrc.m_data to a local does not keep its buffer alive between that read and the subsequent refcount increment. Concurrent release can free it in that interval.

@xezon xezon Oct 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would imply that const AsciiString& stringSrc can be deleted while passed into this function. That makes no sense. It would be the fault of the caller, not the fault of AsciiString.

Check yourself.

@macroscopeapp macroscopeapp Bot Oct 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not require deleting stringSrc: another thread can assign or clear that same source, releasing its current buffer after its pointer is read. The former class lock serialized this exact acquire/release handoff. If concurrent mutation of one AsciiString is now explicitly unsupported, this is an intentional thread-safety behavior change.

if (&stringSrc != this)
{
AsciiStringData* newData = stringSrc.m_data;
if (newData)
++newData->m_refCount;

releaseBuffer();
m_data = stringSrc.m_data;
if (m_data)
++m_data->m_refCount;
m_data = newData;
}
validate();
}
Expand Down
27 changes: 14 additions & 13 deletions Core/GameEngine/Source/Common/System/UnicodeString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
///////////////////////////////////////////////////////////////////////////////
#include "PreRTS.h" // This must go first in EVERY cpp file in the GameEngine

#include "Common/CriticalSection.h"
#include "WWLib/utf8.h"


Expand All @@ -66,7 +65,6 @@ void UnicodeString::validate() const
// -----------------------------------------------------
UnicodeString::UnicodeString(const UnicodeString& stringSrc) : m_data(stringSrc.m_data)
{
ScopedCriticalSection scopedCriticalSection(TheUnicodeStringCriticalSection);
if (m_data)
++m_data->m_refCount;
validate();
Expand Down Expand Up @@ -100,7 +98,8 @@ void UnicodeString::ensureUniqueBufferOfSize(int numCharsNeeded, Bool preserveDa
DEBUG_ASSERTCRASH(numCharsNeeded <= MAX_LEN, ("UnicodeString::ensureUniqueBufferOfSize exceeds max string length %d with requested length %d", MAX_LEN, numCharsNeeded));
int minBytes = sizeof(UnicodeStringData) + numCharsNeeded*sizeof(WideChar);
int actualBytes = TheDynamicMemoryAllocator->getActualAllocationSize(minBytes);
UnicodeStringData* newData = (UnicodeStringData*)TheDynamicMemoryAllocator->allocateBytesDoNotZero(actualBytes, "STR_UnicodeString::ensureUniqueBufferOfSize");
void* rawData = TheDynamicMemoryAllocator->allocateBytesDoNotZero(actualBytes, "STR_UnicodeString::ensureUniqueBufferOfSize");
UnicodeStringData* newData = new(rawData) UnicodeStringData;
newData->m_refCount = 1;
newData->m_numCharsAllocated = (actualBytes - sizeof(UnicodeStringData))/sizeof(WideChar);

Expand Down Expand Up @@ -130,17 +129,19 @@ void UnicodeString::ensureUniqueBufferOfSize(int numCharsNeeded, Bool preserveDa
// -----------------------------------------------------
void UnicodeString::releaseBuffer()
{
ScopedCriticalSection scopedCriticalSection(TheUnicodeStringCriticalSection);

validate();
if (m_data)
{
if (--m_data->m_refCount == 0)
UnicodeStringData* oldData = m_data;
m_data = nullptr;

if (--oldData->m_refCount == 0)
{
TheDynamicMemoryAllocator->freeBytes(m_data);
oldData->~UnicodeStringData();
TheDynamicMemoryAllocator->freeBytes(oldData);
}
m_data = nullptr;
}
validate();
}

// -----------------------------------------------------
Expand All @@ -167,15 +168,15 @@ UnicodeString::UnicodeString(const WideChar* s, int len) : m_data(nullptr)
// -----------------------------------------------------
void UnicodeString::set(const UnicodeString& stringSrc)
{
ScopedCriticalSection scopedCriticalSection(TheUnicodeStringCriticalSection);

validate();
if (&stringSrc != this)
{
UnicodeStringData* newData = stringSrc.m_data;
if (newData)
++newData->m_refCount;

releaseBuffer();
m_data = stringSrc.m_data;
if (m_data)
++m_data->m_refCount;
m_data = newData;
}
validate();
}
Expand Down
2 changes: 0 additions & 2 deletions Generals/Code/GameEngine/Include/Common/CriticalSection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,5 @@ class ScopedCriticalSection

// These should be null on creation then non-null in WinMain or equivalent.
// This allows us to be silently non-threadsafe for WB and other single-threaded apps.
extern CriticalSection *TheAsciiStringCriticalSection;
extern CriticalSection *TheUnicodeStringCriticalSection;
extern CriticalSection *TheDmaCriticalSection;
extern CriticalSection *TheMemoryPoolCriticalSection;
2 changes: 0 additions & 2 deletions Generals/Code/GameEngine/Source/Common/System/CriticalSection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
#include "Common/CriticalSection.h"

// Definitions.
CriticalSection *TheAsciiStringCriticalSection = nullptr;
CriticalSection *TheUnicodeStringCriticalSection = nullptr;
CriticalSection *TheDmaCriticalSection = nullptr;
CriticalSection *TheMemoryPoolCriticalSection = nullptr;

Expand Down
6 changes: 1 addition & 5 deletions Generals/Code/Main/WinMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ static Bool initializeAppWindows( HINSTANCE hInstance, Int nCmdShow, Bool runWin
}

// Necessary to allow memory managers and such to have useful critical sections
static CriticalSection critSec1, critSec2, critSec3, critSec4;
static CriticalSection critSec3, critSec4;

// UnHandledExceptionFilter ===================================================
/** Handler for unhandled win32 exceptions. */
Expand Down Expand Up @@ -806,8 +806,6 @@ Int APIENTRY WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance,
// default in a DevStudio project
//

TheAsciiStringCriticalSection = &critSec1;
TheUnicodeStringCriticalSection = &critSec2;
TheDmaCriticalSection = &critSec3;
TheMemoryPoolCriticalSection = &critSec4;

Expand Down Expand Up @@ -911,8 +909,6 @@ Int APIENTRY WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance,
#ifdef RTS_ENABLE_CRASHDUMP
MiniDumper::shutdownMiniDumper();
#endif
TheAsciiStringCriticalSection = nullptr;
TheUnicodeStringCriticalSection = nullptr;
TheDmaCriticalSection = nullptr;
TheMemoryPoolCriticalSection = nullptr;

Expand Down
2 changes: 0 additions & 2 deletions GeneralsMD/Code/GameEngine/Include/Common/CriticalSection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,5 @@ class ScopedCriticalSection

// These should be null on creation then non-null in WinMain or equivalent.
// This allows us to be silently non-threadsafe for WB and other single-threaded apps.
extern CriticalSection *TheAsciiStringCriticalSection;
extern CriticalSection *TheUnicodeStringCriticalSection;
extern CriticalSection *TheDmaCriticalSection;
Comment thread
greptile-apps[bot] marked this conversation as resolved.
extern CriticalSection *TheMemoryPoolCriticalSection;
2 changes: 0 additions & 2 deletions GeneralsMD/Code/GameEngine/Source/Common/System/CriticalSection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
#include "Common/CriticalSection.h"

// Definitions.
CriticalSection *TheAsciiStringCriticalSection = nullptr;
CriticalSection *TheUnicodeStringCriticalSection = nullptr;
CriticalSection *TheDmaCriticalSection = nullptr;
CriticalSection *TheMemoryPoolCriticalSection = nullptr;

Expand Down
5 changes: 1 addition & 4 deletions GeneralsMD/Code/Main/WinMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ static Bool initializeAppWindows( HINSTANCE hInstance, Int nCmdShow, Bool runWin
}

// Necessary to allow memory managers and such to have useful critical sections
static CriticalSection critSec1, critSec2, critSec3, critSec4;
static CriticalSection critSec3, critSec4;

// UnHandledExceptionFilter ===================================================
/** Handler for unhandled win32 exceptions. */
Expand Down Expand Up @@ -813,8 +813,6 @@ Int APIENTRY WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance,
// default in a DevStudio project
//

TheAsciiStringCriticalSection = &critSec1;
TheUnicodeStringCriticalSection = &critSec2;
TheDmaCriticalSection = &critSec3;
TheMemoryPoolCriticalSection = &critSec4;

Expand Down Expand Up @@ -940,7 +938,6 @@ Int APIENTRY WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance,
#ifdef RTS_ENABLE_CRASHDUMP
MiniDumper::shutdownMiniDumper();
#endif
TheUnicodeStringCriticalSection = nullptr;
TheDmaCriticalSection = nullptr;
TheMemoryPoolCriticalSection = nullptr;

Expand Down
6 changes: 0 additions & 6 deletions Tests/Google/Core/GoogleTestMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,17 @@ class EngineCriticalSections
public:
EngineCriticalSections()
{
TheAsciiStringCriticalSection = &m_asciiString;
TheUnicodeStringCriticalSection = &m_unicodeString;
TheDmaCriticalSection = &m_dma;
TheMemoryPoolCriticalSection = &m_memoryPool;
}

~EngineCriticalSections()
{
TheAsciiStringCriticalSection = nullptr;
TheUnicodeStringCriticalSection = nullptr;
TheDmaCriticalSection = nullptr;
TheMemoryPoolCriticalSection = nullptr;
}

private:
CriticalSection m_asciiString;
CriticalSection m_unicodeString;
CriticalSection m_dma;
CriticalSection m_memoryPool;
};
Expand Down
Loading