[PATCH v2 0/1] MR5379: kernel32: Make MakeCriticalSectionGlobal() a noop.
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. https://gitlab.winehq.org/wine/wine/-/merge_requests/5379
From: Paul Gofman <pgofman(a)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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5379
Do we know that there isn't 9x software that depends on this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65702
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65733
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65734
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65766
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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65767
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65768
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?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65769
Maybe removing the functionality is too radical, I sent alternative way to fix it as https://gitlab.winehq.org/wine/wine/-/merge_requests/5389. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65834
Superseded by 2745228b14d138ea2c7f631c212440ecf7b8f453. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379#note_65868
This merge request was closed by Paul Gofman. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5379
participants (4)
-
Jinoh Kang (＠iamahuman) -
Paul Gofman
-
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)