This fixes a performance regression introduced by 8b3944e1341baaf693927c8b758851d2dbba725a ("ntdll: Only allocate debug info in critical sections with RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO.").
Critical section was using a fallback semaphore path if no debug info is present. That was apparently to support MakeCriticalSectionGlobal() which was deprecated and removed from kernel32 exports around Win2000.
-- v2: kernel32: Make MakeCriticalSectionGlobal() a noop.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/kernel32/sync.c | 8 +---- dlls/ntdll/sync.c | 69 +++++++++----------------------------------- 2 files changed, 15 insertions(+), 62 deletions(-)
diff --git a/dlls/kernel32/sync.c b/dlls/kernel32/sync.c index 53419cd4618..563ca9c400a 100644 --- a/dlls/kernel32/sync.c +++ b/dlls/kernel32/sync.c @@ -126,13 +126,7 @@ BOOL WINAPI UnregisterWait( HANDLE handle ) */ void WINAPI MakeCriticalSectionGlobal( CRITICAL_SECTION *crit ) { - /* let's assume that only one thread at a time will try to do this */ - HANDLE sem = crit->LockSemaphore; - if (!sem) NtCreateSemaphore( &sem, SEMAPHORE_ALL_ACCESS, NULL, 0, 1 ); - crit->LockSemaphore = ConvertToGlobalHandle( sem ); - if (crit->DebugInfo != (void *)(ULONG_PTR)-1) - RtlFreeHeap( GetProcessHeap(), 0, crit->DebugInfo ); - crit->DebugInfo = NULL; + FIXME( "stub.\n" ); }
diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index 00ab614803f..6a9001fe48c 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -161,43 +161,19 @@ static const char *crit_section_get_name( const RTL_CRITICAL_SECTION *crit ) return "?"; }
-static inline HANDLE get_semaphore( RTL_CRITICAL_SECTION *crit ) -{ - HANDLE ret = crit->LockSemaphore; - if (!ret) - { - HANDLE sem; - if (NtCreateSemaphore( &sem, SEMAPHORE_ALL_ACCESS, NULL, 0, 1 )) return 0; - if (!(ret = InterlockedCompareExchangePointer( &crit->LockSemaphore, sem, 0 ))) - ret = sem; - else - NtClose(sem); /* somebody beat us to it */ - } - return ret; -} - -static inline NTSTATUS wait_semaphore( RTL_CRITICAL_SECTION *crit, int timeout ) +static inline NTSTATUS wait_lock( RTL_CRITICAL_SECTION *crit, int timeout ) { LARGE_INTEGER time = {.QuadPart = timeout * (LONGLONG)-10000000}; + LONG *lock = (LONG *)&crit->LockSemaphore;
- /* debug info is cleared by MakeCriticalSectionGlobal */ - if (!crit_section_has_debuginfo( crit )) - { - HANDLE sem = get_semaphore( crit ); - return NtWaitForSingleObject( sem, FALSE, &time ); - } - else + while (!InterlockedCompareExchange( lock, 0, 1 )) { - LONG *lock = (LONG *)&crit->LockSemaphore; - while (!InterlockedCompareExchange( lock, 0, 1 )) - { - static const LONG zero; - /* this may wait longer than specified in case of multiple wake-ups */ - if (RtlWaitOnAddress( lock, &zero, sizeof(LONG), &time ) == STATUS_TIMEOUT) - return STATUS_TIMEOUT; - } - return STATUS_WAIT_0; + static const LONG zero; + /* this may wait longer than specified in case of multiple wake-ups */ + if (RtlWaitOnAddress( lock, &zero, sizeof(LONG), &time ) == STATUS_TIMEOUT) + return STATUS_TIMEOUT; } + return STATUS_WAIT_0; }
/****************************************************************************** @@ -288,11 +264,7 @@ NTSTATUS WINAPI RtlDeleteCriticalSection( RTL_CRITICAL_SECTION *crit ) crit->DebugInfo = NULL; } } - else - { - NtClose( crit->LockSemaphore ); - crit->DebugInfo = NULL; - } + else crit->DebugInfo = NULL; crit->LockSemaphore = 0; return STATUS_SUCCESS; } @@ -315,7 +287,7 @@ NTSTATUS WINAPI RtlpWaitForCriticalSection( RTL_CRITICAL_SECTION *crit )
for (;;) { - NTSTATUS status = wait_semaphore( crit, timeout ); + NTSTATUS status = wait_lock( crit, timeout );
if (status == STATUS_WAIT_0) break;
@@ -334,23 +306,10 @@ NTSTATUS WINAPI RtlpWaitForCriticalSection( RTL_CRITICAL_SECTION *crit ) */ NTSTATUS WINAPI RtlpUnWaitCriticalSection( RTL_CRITICAL_SECTION *crit ) { - NTSTATUS ret; - - /* debug info is cleared by MakeCriticalSectionGlobal */ - if (!crit_section_has_debuginfo( crit )) - { - HANDLE sem = get_semaphore( crit ); - ret = NtReleaseSemaphore( sem, 1, NULL ); - } - else - { - LONG *lock = (LONG *)&crit->LockSemaphore; - InterlockedExchange( lock, 1 ); - RtlWakeAddressSingle( lock ); - ret = STATUS_SUCCESS; - } - if (ret) RtlRaiseStatus( ret ); - return ret; + LONG *lock = (LONG *)&crit->LockSemaphore; + InterlockedExchange( lock, 1 ); + RtlWakeAddressSingle( lock ); + return STATUS_SUCCESS; }
Do we know that there isn't 9x software that depends on this?
I don't know how to make sure. But I think it is not much likely used because:
- I am not finding anything like that in MS compatibility shim administrator; - I am not seeing any mention of it in Wine bugzilla; - That was probably an undocumented function used internally in Windows (and Wine) but its use was removed since then.
Then, I am not removing it from exports in this patch. If anything using it and depending on its effect is discovered we will see the FIXME.
On Fri Mar 22 04:00:34 2024 +0000, Zebediah Figura wrote:
Do we know that there isn't 9x software that depends on this?
Apparently DirectX 9.0C has it: https://jira.reactos.org/si/jira.issueviews:issue-html/CORE-13541/CORE-13541...
There is an anecdote that some configuration needs MakeCriticalSectionGlobal() to run Age of Empires III, which has 29 bug reports in WineHQ bugzilla.
IIUC LockSemaphore is either 0 or 1 or a valid handle. Perhaps IMHO we want to just change the logic not to rely on DebugInfo, testing LockSemaphore value instead?
On Fri Mar 22 04:04:56 2024 +0000, Jinoh Kang wrote:
IIUC LockSemaphore is either 0 or 1 or a valid handle. Perhaps IMHO we want to just change the logic not to rely on DebugInfo, testing LockSemaphore value instead?
Didn't see the first post. Sorry for the noise.
On Fri Mar 22 04:03:44 2024 +0000, Jinoh Kang wrote:
Apparently DirectX 9.0C has it: https://jira.reactos.org/si/jira.issueviews:issue-html/CORE-13541/CORE-13541... There is an [anecdote][] that some configuration needs MakeCriticalSectionGlobal() to run Age of Empires III, which has 29 bug reports in WineHQ bugzilla. [anecdote]: https://aoe3.heavengames.com/cgi-bin/forums/display.cgi?action=ct&f=11,23809,2820,all
Does any of that need the function to do what it is currently doing, or just be present?
Maybe removing the functionality is too radical, I sent alternative way to fix it as https://gitlab.winehq.org/wine/wine/-/merge_requests/5389.
Superseded by 2745228b14d138ea2c7f631c212440ecf7b8f453.
This merge request was closed by Paul Gofman.
-
Jinoh Kang (＠iamahuman) -
Paul Gofman
-
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)