[PATCH 0/1] MR5379: kernel32: Make MakeCriticalSectionGlobal() a noop.

[Paul Gofman (＠gofman)]

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.

[PATCH 1/1] kernel32: Make MakeCriticalSectionGlobal() a noop.

[Paul Gofman]

From: Paul Gofman

---
 dlls/kernel32/sync.c |  8 +-----
 dlls/ntdll/sync.c    | 63 +++++++++-----------------------------------
 2 files changed, 14 insertions(+), 57 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..574d351a585 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 ))
+    while (!InterlockedCompareExchange( lock, 0, 1 ))
     {
-        HANDLE sem = get_semaphore( crit );
-        return NtWaitForSingleObject( sem, FALSE, &time );
-    }
-    else
-    {
-        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;
 }

/******************************************************************************
@@ -315,7 +291,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 +310,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;
 }

[Paul Gofman (＠gofman)]

If I am missing something and there is some rationale under keeping MakeCriticalSectionGlobal with semaphore creation around, another way to fix that is probably use the fact that valid handles are multiples of 4 while for fast path LockSemaphore can only be 0 or 1, and distinguish whether the semaphore is used by LockSemaphore value.