Use ReentrantLock for PhysicalMemory synchronization

Don't use volatile in DirectMemoryAccessors.isInitialized and don't use synchronization as the outcome of those races should amount to the same values.
graalvm · Sep 14, 2023 · 9d5b338 · 9d5b338
1 parent 4c7cafe
commit 9d5b338
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 27 deletions.
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/heap/PhysicalMemory.java
@@ -26,6 +26,7 @@
 
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.word.UnsignedWord;
@@ -59,6 +60,7 @@ default boolean hasSize() {
 
     private static final CountDownLatch CACHED_SIZE_AVAIL_LATCH = new CountDownLatch(1);
     private static final AtomicInteger INITIALIZING = new AtomicInteger(0);
+    private static final ReentrantLock LOCK = new ReentrantLock();
     private static final UnsignedWord UNSET_SENTINEL = UnsignedUtils.MAX_VALUE;
     private static UnsignedWord cachedSize = UNSET_SENTINEL;
 
@@ -90,9 +92,9 @@ public static UnsignedWord size() {
             throw VMError.shouldNotReachHere("Accessing the physical memory size may require allocation and synchronization");
         }
 
-        synchronized (INITIALIZING) {
+        LOCK.lock();
+        try {
             if (!isInitialized()) {
-                VMError.guarantee(INITIALIZING.get() <= 1, "Must not initialize twice");
                 if (isInitializing()) {
                     /*
                      * Recursive initializations need to wait for the one initializing thread to
@@ -110,23 +112,22 @@ public static UnsignedWord size() {
                     }
                 }
                 INITIALIZING.incrementAndGet();
-                try {
-                    long memoryLimit = SubstrateOptions.MaxRAM.getValue();
-                    if (memoryLimit > 0) {
-                        cachedSize = WordFactory.unsigned(memoryLimit);
-                    } else {
-                        memoryLimit = Containers.memoryLimitInBytes();
-                        cachedSize = memoryLimit > 0
-                                        ? WordFactory.unsigned(memoryLimit)
-                                        : ImageSingletons.lookup(PhysicalMemorySupport.class).size();
-                    }
-                    // Now that we have set the cachedSize let other threads know it's
-                    // available to use.
-                    CACHED_SIZE_AVAIL_LATCH.countDown();
-                } finally {
-                    INITIALIZING.incrementAndGet();
+                long memoryLimit = SubstrateOptions.MaxRAM.getValue();
+                if (memoryLimit > 0) {
+                    cachedSize = WordFactory.unsigned(memoryLimit);
+                } else {
+                    memoryLimit = Containers.memoryLimitInBytes();
+                    cachedSize = memoryLimit > 0
+                                    ? WordFactory.unsigned(memoryLimit)
+                                    : ImageSingletons.lookup(PhysicalMemorySupport.class).size();
                 }
+                // Now that we have set the cachedSize let other threads know it's
+                // available to use.
+                INITIALIZING.incrementAndGet();
+                CACHED_SIZE_AVAIL_LATCH.countDown();
             }
+        } finally {
+            LOCK.unlock();
         }
 
         return cachedSize;

diff --git a/...tevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java b/...tevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java
@@ -74,16 +74,10 @@ public static ClassLoader latestUserDefinedLoader0() {
     private static boolean pageAlignDirectMemory;
 
     @Alias //
-    @SuppressWarnings("unused")
-    public static void initLevel(int newVal) {
-        throw VMError.shouldNotReachHere("This is an alias to the method in the target class, so this code is unreachable");
-    }
+    public static native void initLevel(int newVal);
 
     @Alias //
-    @SuppressWarnings("unused")
-    public static int initLevel() {
-        return -1; // Aliased code. Should not reach here
-    }
+    public static native int initLevel();
 }
 
 final class DirectMemoryAccessors {
@@ -95,7 +89,7 @@ final class DirectMemoryAccessors {
      * size. That can only be done once PhysicalMemory init completed. We'd introduce a cycle
      * otherwise.
      */
-    private static volatile boolean isInitialized;
+    private static boolean isInitialized;
     private static final AtomicInteger INIT_COUNT = new AtomicInteger();
     private static final long STATIC_DIRECT_MEMORY_AMOUNT = 25 * 1024 * 1024;
     private static long directMemory;
@@ -107,7 +101,7 @@ static long getDirectMemory() {
         return directMemory;
     }
 
-    private static synchronized void initialize() {
+    private static void initialize() {
         if (INIT_COUNT.get() == 2) {
             /*
              * Shouldn't really happen, but safeguard for recursive init anyway