8248657: Windows: strengthening in ThreadCritical regarding memory model

luhenry · tstuefe · commit 6af643e5a1f9 · 2020-07-29T10:38:28.000+02:00
Reviewed-by: dholmes, kbarrett, aph, stuefe
diff --git a/src/hotspot/os/windows/threadCritical_windows.cpp b/src/hotspot/os/windows/threadCritical_windows.cpp
@@ -35,10 +35,10 @@
 // See threadCritical.hpp for details of this class.
 //
 
-static bool initialized = false;
-static volatile int lock_count = -1;
+static INIT_ONCE initialized = INIT_ONCE_STATIC_INIT;
+static int lock_count = 0;
 static HANDLE lock_event;
-static DWORD lock_owner = -1;
+static DWORD lock_owner = 0;
 
 //
 // Note that Microsoft's critical region code contains a race
@@ -51,48 +51,36 @@ static DWORD lock_owner = -1;
 // and found them ~30 times slower than the critical region code.
 //
 
+static BOOL WINAPI initialize(PINIT_ONCE InitOnce, PVOID Parameter, PVOID *Context) {
+  lock_event = CreateEvent(NULL, false, true, NULL);
+  assert(lock_event != NULL, "unexpected return value from CreateEvent");
+  return true;
+}
+
 ThreadCritical::ThreadCritical() {
-  DWORD current_thread = GetCurrentThreadId();
+  InitOnceExecuteOnce(&initialized, &initialize, NULL, NULL);
 
+  DWORD current_thread = GetCurrentThreadId();
   if (lock_owner != current_thread) {
     // Grab the lock before doing anything.
-    while (Atomic::cmpxchg(&lock_count, -1, 0) != -1) {
-      if (initialized) {
-        DWORD ret = WaitForSingleObject(lock_event,  INFINITE);
-        assert(ret == WAIT_OBJECT_0, "unexpected return value from WaitForSingleObject");
-      }
-    }
-
-    // Make sure the event object is allocated.
-    if (!initialized) {
-      // Locking will not work correctly unless this is autoreset.
-      lock_event = CreateEvent(NULL, false, false, NULL);
-      initialized = true;
-    }
-
-    assert(lock_owner == -1, "Lock acquired illegally.");
+    DWORD ret = WaitForSingleObject(lock_event,  INFINITE);
+    assert(ret == WAIT_OBJECT_0, "unexpected return value from WaitForSingleObject");
     lock_owner = current_thread;
-  } else {
-    // Atomicity isn't required. Bump the recursion count.
-    lock_count++;
   }
-
-  assert(lock_owner == GetCurrentThreadId(), "Lock acquired illegally.");
+  // Atomicity isn't required. Bump the recursion count.
+  lock_count++;
 }
 
 ThreadCritical::~ThreadCritical() {
   assert(lock_owner == GetCurrentThreadId(), "unlock attempt by wrong thread");
   assert(lock_count >= 0, "Attempt to unlock when already unlocked");
 
+  lock_count--;
   if (lock_count == 0) {
     // We're going to unlock
-    lock_owner = -1;
-    lock_count = -1;
+    lock_owner = 0;
     // No lost wakeups, lock_event stays signaled until reset.
     DWORD ret = SetEvent(lock_event);
     assert(ret != 0, "unexpected return value from SetEvent");
-  } else {
-    // Just unwinding a recursive lock;
-    lock_count--;
   }
 }
