From 1ae2dc011c6c74f25ef49e5cc5bd50d814545fff Mon Sep 17 00:00:00 2001 From: Severin Gehwolf Date: Wed, 30 Aug 2023 16:54:24 +0200 Subject: [PATCH 01/11] Break the initialization cycle in NIO/cgroups code First, we use a separate accessor for page-alignedness as it doesn't need the more sophisticated initialization of the directMemory field. Next, ensure PhysicalMemory initialization is serialized and when it is, set directMemory to a static value so that the container code can finish initialization without introducing a cyle. The final directMemory value based on the heap size is then published to JDK code by setting the VM init level to 1. Therefore, application code would use the non-static value as the upper bound. Closes: #556 --- .../oracle/svm/core/heap/PhysicalMemory.java | 51 ++++++-- .../core/jdk/Target_jdk_internal_misc_VM.java | 118 ++++++++++++++---- 2 files changed, 138 insertions(+), 31 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 index 4fe58e816ae6..fc557390a017 100644 --- 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 @@ -24,6 +24,10 @@ */ package com.oracle.svm.core.heap; +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; import org.graalvm.word.WordFactory; @@ -55,13 +59,23 @@ default boolean hasSize() { UnsignedWord size(); } + 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; @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true) public static boolean isInitialized() { - return cachedSize != UNSET_SENTINEL; + return INITIALIZING.get() > 1; + } + + /** + * + * @return {@code true} when PhycialMemory.size() is still initializing + */ + private static boolean isInitializing() { + return INITIALIZING.get() == 1; } /** @@ -80,13 +94,26 @@ public static UnsignedWord size() { throw VMError.shouldNotReachHere("Accessing the physical memory size may require allocation and synchronization"); } - if (!isInitialized()) { - /* - * Multiple threads can race to initialize the cache. This is OK because all of them - * will (most-likely) compute the same value. - */ - INITIALIZING.incrementAndGet(); - try { + LOCK.lock(); + try { + if (!isInitialized()) { + if (isInitializing()) { + /* + * Recursive initializations need to wait for the one initializing thread to + * finish so as to get correct reads of the cachedSize value. + */ + try { + boolean expired = !CACHED_SIZE_AVAIL_LATCH.await(1L, TimeUnit.SECONDS); + if (expired) { + throw new InternalError("Expired latch!"); + } + VMError.guarantee(cachedSize != UNSET_SENTINEL, "Expected chached size to be set"); + return cachedSize; + } catch (InterruptedException e) { + throw VMError.shouldNotReachHere("Interrupt on countdown latch!"); + } + } + INITIALIZING.incrementAndGet(); long memoryLimit = SubstrateOptions.MaxRAM.getValue(); if (memoryLimit > 0) { cachedSize = WordFactory.unsigned(memoryLimit); @@ -96,9 +123,13 @@ public static UnsignedWord size() { ? WordFactory.unsigned(memoryLimit) : ImageSingletons.lookup(PhysicalMemorySupport.class).size(); } - } finally { - INITIALIZING.decrementAndGet(); + // 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/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java index a86d95a6755f..c14c745afed4 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java @@ -25,6 +25,7 @@ package com.oracle.svm.core.jdk; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import com.oracle.svm.core.NeverInline; import com.oracle.svm.core.SubstrateOptions; @@ -35,7 +36,9 @@ import com.oracle.svm.core.annotate.RecomputeFieldValue.Kind; import com.oracle.svm.core.annotate.Substitute; import com.oracle.svm.core.annotate.TargetClass; +import com.oracle.svm.core.heap.PhysicalMemory; import com.oracle.svm.core.snippets.KnownIntrinsics; +import com.oracle.svm.core.util.VMError; import jdk.internal.misc.Unsafe; @@ -67,36 +70,44 @@ public static ClassLoader latestUserDefinedLoader0() { @Alias @InjectAccessors(DirectMemoryAccessors.class) // private static long directMemory; - @Alias @InjectAccessors(DirectMemoryAccessors.class) // + @Alias @InjectAccessors(PageAlignDirectMemoryAccessors.class) // private static boolean pageAlignDirectMemory; + + @Alias // + public static native void initLevel(int newVal); + + @Alias // + public static native int initLevel(); } final class DirectMemoryAccessors { /* - * Not volatile to avoid a memory barrier when reading the values. Instead, an explicit barrier - * is inserted when writing the values. + * Full initialization is two-staged. First, we init directMemory to a static value (25MB) so + * that initialization of PhysicalMemory has a chance to finish. At that point isInintialized + * will be false, since we need to (potentially) set the value to the actual configured heap + * size. That can only be done once PhysicalMemory init completed. We'd introduce a cycle + * otherwise. */ - private static boolean initialized; - + 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; - private static boolean pageAlignDirectMemory; static long getDirectMemory() { - if (!initialized) { + if (!isInitialized) { initialize(); } return directMemory; } - static boolean getPageAlignDirectMemory() { - if (!initialized) { - initialize(); - } - return pageAlignDirectMemory; - } - private static void initialize() { + if (INIT_COUNT.get() == 2) { + /* + * Shouldn't really happen, but safeguard for recursive init anyway + */ + return; + } /* * The JDK method VM.saveAndRemoveProperties looks at the system property * "sun.nio.MaxDirectMemorySize". However, that property is always set by the Java HotSpot @@ -107,17 +118,82 @@ private static void initialize() { if (newDirectMemory == 0) { /* * No value explicitly specified. The default in the JDK in this case is the maximum - * heap size. + * heap size. However, we cannot rely on Runtime.maxMemory() until PhysicalMemory has + * fully initialized. Runtime.maxMemory() has a dependency on PhysicalMemory.size() + * which in turn depends on container support which might use NIO. To avoid this cycle, + * we first initialize the 'directMemory' field to an arbitrary value (25MB), and only + * use the Runtime.maxMemory() API once PhysicalMemory has fully initialized. + */ + if (!PhysicalMemory.isInitialized()) { + /* + * While initializing physical memory we might end up back here with an INIT_COUNT + * of 1, since we read the directMemory field during container support code + * execution which runs when PhysicalMemory is still initializing. + */ + VMError.guarantee(INIT_COUNT.get() <= 1, "Initial run needs to have init count 0 or 1"); + newDirectMemory = STATIC_DIRECT_MEMORY_AMOUNT; // Static value during initialization + INIT_COUNT.incrementAndGet(); + } else { + VMError.guarantee(INIT_COUNT.get() <= 1, "Runtime.maxMemory() invariant"); + /* + * Once we know PhysicalMemory has been properly initialized we can use + * Runtime.maxMemory(). Note that we might end up in this branch for code explicitly + * using the JDK cgroups code. At that point PhysicalMemory has likely been + * initialized. + */ + INIT_COUNT.incrementAndGet(); + newDirectMemory = Runtime.getRuntime().maxMemory(); + } + } else { + /* + * For explicitly set direct memory we are done */ - newDirectMemory = Runtime.getRuntime().maxMemory(); + Unsafe.getUnsafe().storeFence(); + directMemory = newDirectMemory; + isInitialized = true; + if (Target_jdk_internal_misc_VM.initLevel() < 1) { + // only the first accessor needs to set this + Target_jdk_internal_misc_VM.initLevel(1); + } + return; } + VMError.guarantee(newDirectMemory > 0, "New direct memory should be initialized"); - /* - * The initialization is not synchronized, so multiple threads can race. Usually this will - * lead to the same value, unless the runtime options are modified concurrently - which is - * possible but not a case we care about. - */ + Unsafe.getUnsafe().storeFence(); directMemory = newDirectMemory; + if (PhysicalMemory.isInitialized()) { + /* + * Complete initialization hand-shake once PhysicalMemory is properly initialized. Also + * set the VM init level to 1 so as to provoke the NIO code to re-set the internal + * MAX_MEMORY field. + */ + isInitialized = true; + if (Target_jdk_internal_misc_VM.initLevel() < 1) { + // only the first accessor needs to set this + Target_jdk_internal_misc_VM.initLevel(1); + } + } + } +} + +final class PageAlignDirectMemoryAccessors { + + /* + * Not volatile to avoid a memory barrier when reading the values. Instead, an explicit barrier + * is inserted when writing the values. + */ + private static boolean initialized; + + private static boolean pageAlignDirectMemory; + + static boolean getPageAlignDirectMemory() { + if (!initialized) { + initialize(); + } + return pageAlignDirectMemory; + } + + private static void initialize() { pageAlignDirectMemory = Boolean.getBoolean("sun.nio.PageAlignDirectMemory"); /* Ensure values are published to other threads before marking fields as initialized. */ From 318f1083a75735a19f162f586ff2f815a771b1b1 Mon Sep 17 00:00:00 2001 From: Severin Gehwolf Date: Wed, 4 Oct 2023 18:37:11 +0200 Subject: [PATCH 02/11] Fix race with double init --- .../com/oracle/svm/core/heap/PhysicalMemory.java | 2 +- .../core/jdk/Target_jdk_internal_misc_VM.java | 16 +++++++++------- 2 files changed, 10 insertions(+), 8 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 index fc557390a017..8b4663ab0ef1 100644 --- 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 @@ -107,7 +107,7 @@ public static UnsignedWord size() { if (expired) { throw new InternalError("Expired latch!"); } - VMError.guarantee(cachedSize != UNSET_SENTINEL, "Expected chached size to be set"); + VMError.guarantee(cachedSize != UNSET_SENTINEL, "Expected cached size to be set"); return cachedSize; } catch (InterruptedException e) { throw VMError.shouldNotReachHere("Interrupt on countdown latch!"); diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java index c14c745afed4..058880077c5c 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java @@ -90,6 +90,8 @@ final class DirectMemoryAccessors { * otherwise. */ private static boolean isInitialized; + private static final int INITIALIZING = 1; + private static final int INITIALIZED = 2; private static final AtomicInteger INIT_COUNT = new AtomicInteger(); private static final long STATIC_DIRECT_MEMORY_AMOUNT = 25 * 1024 * 1024; private static long directMemory; @@ -102,9 +104,9 @@ static long getDirectMemory() { } private static void initialize() { - if (INIT_COUNT.get() == 2) { + if (INIT_COUNT.get() == INITIALIZED) { /* - * Shouldn't really happen, but safeguard for recursive init anyway + * Safeguard for recursive init */ return; } @@ -130,18 +132,18 @@ private static void initialize() { * of 1, since we read the directMemory field during container support code * execution which runs when PhysicalMemory is still initializing. */ - VMError.guarantee(INIT_COUNT.get() <= 1, "Initial run needs to have init count 0 or 1"); + VMError.guarantee(INIT_COUNT.get() <= INITIALIZING, "Initial run needs to have init count 0 or 1"); newDirectMemory = STATIC_DIRECT_MEMORY_AMOUNT; // Static value during initialization - INIT_COUNT.incrementAndGet(); + INIT_COUNT.setRelease(INITIALIZING); } else { - VMError.guarantee(INIT_COUNT.get() <= 1, "Runtime.maxMemory() invariant"); + VMError.guarantee(INIT_COUNT.get() <= INITIALIZING, "Runtime.maxMemory() invariant"); /* * Once we know PhysicalMemory has been properly initialized we can use * Runtime.maxMemory(). Note that we might end up in this branch for code explicitly * using the JDK cgroups code. At that point PhysicalMemory has likely been * initialized. */ - INIT_COUNT.incrementAndGet(); + INIT_COUNT.setRelease(INITIALIZED); newDirectMemory = Runtime.getRuntime().maxMemory(); } } else { @@ -161,7 +163,7 @@ private static void initialize() { Unsafe.getUnsafe().storeFence(); directMemory = newDirectMemory; - if (PhysicalMemory.isInitialized()) { + if (PhysicalMemory.isInitialized() && INITIALIZED == INIT_COUNT.get()) { /* * Complete initialization hand-shake once PhysicalMemory is properly initialized. Also * set the VM init level to 1 so as to provoke the NIO code to re-set the internal From ed3382fecec1f6ac17bfb8a9efdbf436c7b588bb Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Mon, 18 Sep 2023 13:31:29 +0200 Subject: [PATCH 03/11] read elimination: any only kills mutable locations --- .../phases/ea/ReadEliminationBlockState.java | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/ReadEliminationBlockState.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/ReadEliminationBlockState.java index 6b3e1b0fdc64..00007a86bed1 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/ReadEliminationBlockState.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/ReadEliminationBlockState.java @@ -193,22 +193,31 @@ public ValueNode getCacheEntry(CacheEntry identifier) { public void killReadCache(LocationIdentity identity, ValueNode index, ValueNode array) { if (identity.isAny()) { - // ANY aliases with every other location - readCache.clear(); - return; - } - Iterator> iterator = readCache.getKeys().iterator(); - while (iterator.hasNext()) { - CacheEntry entry = iterator.next(); - /* - * We cover multiple cases here but in general index and array can only be !=null for - * indexed nodes thus the location identity of other accesses (field and object - * locations) will never be the same and will never alias with array accesses. - * - * Unsafe accesses will alias if they are writing to any location. + /** + * Kill all mutable locations. */ - if (entry.conflicts(identity, index, array)) { - iterator.remove(); + Iterator> iterator = readCache.getKeys().iterator(); + while (iterator.hasNext()) { + CacheEntry entry = iterator.next(); + if (entry.getIdentity().isMutable()) { + iterator.remove(); + } + } + return; + } else { + Iterator> iterator = readCache.getKeys().iterator(); + while (iterator.hasNext()) { + CacheEntry entry = iterator.next(); + /* + * We cover multiple cases here but in general index and array can only be !=null + * for indexed nodes thus the location identity of other accesses (field and object + * locations) will never be the same and will never alias with array accesses. + * + * Unsafe accesses will alias if they are writing to any location. + */ + if (entry.conflicts(identity, index, array)) { + iterator.remove(); + } } } } From 7192a973c73176e20e64399bf5741d014a83aac2 Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Mon, 18 Sep 2023 17:20:15 +0200 Subject: [PATCH 04/11] low tier: export canon without gvn --- .../src/org/graalvm/compiler/core/phases/LowTier.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/phases/LowTier.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/phases/LowTier.java index 4b49620c20b8..a8b0a3926af9 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/phases/LowTier.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/phases/LowTier.java @@ -57,10 +57,12 @@ static class Options { } + private final CanonicalizerPhase canonicalizerWithoutGVN; + @SuppressWarnings("this-escape") public LowTier(OptionValues options) { CanonicalizerPhase canonicalizer = CanonicalizerPhase.create(); - CanonicalizerPhase canonicalizerWithoutGVN = canonicalizer.copyWithoutGVN(); + this.canonicalizerWithoutGVN = canonicalizer.copyWithoutGVN(); if (Options.ProfileCompiledMethods.getValue(options)) { appendPhase(new ProfileCompiledMethodsPhase()); @@ -92,4 +94,8 @@ public LowTier(OptionValues options) { appendPhase(new SchedulePhase(SchedulePhase.SchedulingStrategy.LATEST_OUT_OF_LOOPS)); } + + public CanonicalizerPhase getCanonicalizerWithoutGVN() { + return canonicalizerWithoutGVN; + } } From e145a3de9953d9f951cde81a03be61386b9067ac Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Mon, 18 Sep 2023 17:20:36 +0200 Subject: [PATCH 05/11] canon: refactorings --- .../compiler/phases/common/CanonicalizerPhase.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java index c37db6ca3784..66ea2ce27835 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java @@ -156,6 +156,10 @@ protected CanonicalizerPhase(CustomSimplification customSimplification, EnumSet< this.features = features; } + public EnumSet getFeatures() { + return features; + } + public CanonicalizerPhase copyWithCustomSimplification(CustomSimplification newSimplification) { return new CanonicalizerPhase(newSimplification, features); } @@ -502,12 +506,17 @@ private boolean processNode(Node node, Tool tool) { } } // Perform GVN after possibly inferring the stamp since a stale stamp will inhibit GVN. - if (features.contains(GVN) && tryGlobalValueNumbering(node, nodeClass)) { + if (tryGVN(node) && tryGlobalValueNumbering(node, nodeClass)) { return true; } return false; } + @SuppressWarnings("unused") + public boolean tryGVN(Node n) { + return features.contains(GVN); + } + public boolean tryGlobalValueNumbering(Node node, NodeClass nodeClass) { if (nodeClass.valueNumberable()) { Node newNode = node.graph().findDuplicate(node); From 7ccdf1f500748f0793d947c04c16630510d1b7c6 Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Thu, 21 Sep 2023 12:21:31 +0200 Subject: [PATCH 06/11] canon: refactorings --- .../compiler/phases/common/CanonicalizerPhase.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java index 66ea2ce27835..d7be7399d1c4 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java @@ -506,18 +506,13 @@ private boolean processNode(Node node, Tool tool) { } } // Perform GVN after possibly inferring the stamp since a stale stamp will inhibit GVN. - if (tryGVN(node) && tryGlobalValueNumbering(node, nodeClass)) { + if (features.contains(GVN) && tryGlobalValueNumbering(node, nodeClass)) { return true; } return false; } - @SuppressWarnings("unused") - public boolean tryGVN(Node n) { - return features.contains(GVN); - } - - public boolean tryGlobalValueNumbering(Node node, NodeClass nodeClass) { + public static boolean gvn(Node node, NodeClass nodeClass) { if (nodeClass.valueNumberable()) { Node newNode = node.graph().findDuplicate(node); if (newNode != null) { @@ -531,6 +526,10 @@ public boolean tryGlobalValueNumbering(Node node, NodeClass nodeClass) { return false; } + public boolean tryGlobalValueNumbering(Node node, NodeClass nodeClass) { + return gvn(node, nodeClass); + } + private static AutoCloseable getCanonicalizeableContractAssertion(Node node) { boolean needsAssertion = false; assert (needsAssertion = true) == true; From 8cf5fb8e8262d726c9d35f307b0caf6ccb990e16 Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Tue, 3 Oct 2023 13:51:38 +0200 Subject: [PATCH 07/11] assert no gvn is done after fixing reads --- .../graalvm/compiler/core/phases/LowTier.java | 15 +++++++---- .../phases/common/CanonicalizerPhase.java | 1 + .../phases/ea/ReadEliminationBlockState.java | 27 +++++++++---------- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/phases/LowTier.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/phases/LowTier.java index a8b0a3926af9..8cdf06d6ece1 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/phases/LowTier.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/core/phases/LowTier.java @@ -58,19 +58,20 @@ static class Options { } private final CanonicalizerPhase canonicalizerWithoutGVN; + private final CanonicalizerPhase canonicalizerWithGVN; @SuppressWarnings("this-escape") public LowTier(OptionValues options) { - CanonicalizerPhase canonicalizer = CanonicalizerPhase.create(); - this.canonicalizerWithoutGVN = canonicalizer.copyWithoutGVN(); + this.canonicalizerWithGVN = CanonicalizerPhase.create(); + this.canonicalizerWithoutGVN = canonicalizerWithGVN.copyWithoutGVN(); if (Options.ProfileCompiledMethods.getValue(options)) { appendPhase(new ProfileCompiledMethodsPhase()); } - appendPhase(new LowTierLoweringPhase(canonicalizer)); + appendPhase(new LowTierLoweringPhase(canonicalizerWithGVN)); - appendPhase(new ExpandLogicPhase(canonicalizer)); + appendPhase(new ExpandLogicPhase(canonicalizerWithGVN)); appendPhase(new FixReadsPhase(true, new SchedulePhase(GraalOptions.StressTestEarlyReads.getValue(options) ? SchedulingStrategy.EARLIEST : SchedulingStrategy.LATEST_OUT_OF_LOOPS_IMPLICIT_NULL_CHECKS))); @@ -95,7 +96,11 @@ public LowTier(OptionValues options) { appendPhase(new SchedulePhase(SchedulePhase.SchedulingStrategy.LATEST_OUT_OF_LOOPS)); } - public CanonicalizerPhase getCanonicalizerWithoutGVN() { + public final CanonicalizerPhase getCanonicalizerWithoutGVN() { return canonicalizerWithoutGVN; } + + public final CanonicalizerPhase getCanonicalizer() { + return canonicalizerWithGVN; + } } diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java index d7be7399d1c4..a74b48874db1 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java @@ -527,6 +527,7 @@ public static boolean gvn(Node node, NodeClass nodeClass) { } public boolean tryGlobalValueNumbering(Node node, NodeClass nodeClass) { + assert ((StructuredGraph) node.graph()).getGraphState().isBeforeStage(StageFlag.FIXED_READS) : "GVN must not occur after fixing reads, trying to gvn " + node + " for graph " + node.graph(); return gvn(node, nodeClass); } diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/ReadEliminationBlockState.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/ReadEliminationBlockState.java index 00007a86bed1..624e8a88eb41 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/ReadEliminationBlockState.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/virtual/phases/ea/ReadEliminationBlockState.java @@ -204,20 +204,19 @@ public void killReadCache(LocationIdentity identity, ValueNode index, ValueNode } } return; - } else { - Iterator> iterator = readCache.getKeys().iterator(); - while (iterator.hasNext()) { - CacheEntry entry = iterator.next(); - /* - * We cover multiple cases here but in general index and array can only be !=null - * for indexed nodes thus the location identity of other accesses (field and object - * locations) will never be the same and will never alias with array accesses. - * - * Unsafe accesses will alias if they are writing to any location. - */ - if (entry.conflicts(identity, index, array)) { - iterator.remove(); - } + } + Iterator> iterator = readCache.getKeys().iterator(); + while (iterator.hasNext()) { + CacheEntry entry = iterator.next(); + /* + * We cover multiple cases here but in general index and array can only be !=null for + * indexed nodes thus the location identity of other accesses (field and object + * locations) will never be the same and will never alias with array accesses. + * + * Unsafe accesses will alias if they are writing to any location. + */ + if (entry.conflicts(identity, index, array)) { + iterator.remove(); } } } From aa0b8b2b1b54f824f4aabaeea11aea88c0bcc9d9 Mon Sep 17 00:00:00 2001 From: David Leopoldseder Date: Wed, 4 Oct 2023 11:54:45 +0200 Subject: [PATCH 08/11] add stage flag for pr expand --- .../src/org/graalvm/compiler/nodes/GraphState.java | 1 + .../graalvm/compiler/phases/common/CanonicalizerPhase.java | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/GraphState.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/GraphState.java index 7bcfba83d25e..6744af60a81c 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/GraphState.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/GraphState.java @@ -625,6 +625,7 @@ public enum StageFlag { VECTOR_LOWERING, EXPAND_LOGIC, FIXED_READS, + PARTIAL_REDUNDANCY_SCHEDULE, ADDRESS_LOWERING, FINAL_CANONICALIZATION, TARGET_VECTOR_LOWERING, diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java index a74b48874db1..fd37116f4080 100644 --- a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java +++ b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/CanonicalizerPhase.java @@ -527,7 +527,9 @@ public static boolean gvn(Node node, NodeClass nodeClass) { } public boolean tryGlobalValueNumbering(Node node, NodeClass nodeClass) { - assert ((StructuredGraph) node.graph()).getGraphState().isBeforeStage(StageFlag.FIXED_READS) : "GVN must not occur after fixing reads, trying to gvn " + node + " for graph " + node.graph(); + assert ((StructuredGraph) node.graph()).getGraphState().isBeforeStage(StageFlag.PARTIAL_REDUNDANCY_SCHEDULE) : "GVN must not occur after expanding partially redundant nodes, trying to gvn " + + node + + " for graph " + node.graph(); return gvn(node, nodeClass); } From af74d9f0f6d0304d6bad7408c1bc92a6e4788c57 Mon Sep 17 00:00:00 2001 From: Christian Haeubl Date: Thu, 5 Oct 2023 13:53:27 +0200 Subject: [PATCH 09/11] Break the initialization cycle in NIO/cgroups code in a simpler and more thread-safe way. --- .../oracle/svm/core/heap/PhysicalMemory.java | 62 +++-------- .../core/jdk/Target_jdk_internal_misc_VM.java | 103 +++++------------- 2 files changed, 43 insertions(+), 122 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 index 8b4663ab0ef1..0e1b21cb5dcc 100644 --- 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 @@ -24,8 +24,6 @@ */ package com.oracle.svm.core.heap; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.ReentrantLock; import org.graalvm.nativeimage.ImageSingletons; @@ -35,7 +33,6 @@ import com.oracle.svm.core.Containers; import com.oracle.svm.core.SubstrateOptions; import com.oracle.svm.core.Uninterruptible; -import com.oracle.svm.core.jdk.UninterruptibleUtils.AtomicInteger; import com.oracle.svm.core.stack.StackOverflowCheck; import com.oracle.svm.core.thread.PlatformThreads; import com.oracle.svm.core.thread.VMOperation; @@ -59,23 +56,17 @@ default boolean hasSize() { UnsignedWord size(); } - 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; @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true) public static boolean isInitialized() { - return INITIALIZING.get() > 1; + return cachedSize != UNSET_SENTINEL; } - /** - * - * @return {@code true} when PhycialMemory.size() is still initializing - */ - private static boolean isInitializing() { - return INITIALIZING.get() == 1; + public static boolean isInitializationInProgress() { + return LOCK.isHeldByCurrentThread(); } /** @@ -94,42 +85,23 @@ public static UnsignedWord size() { throw VMError.shouldNotReachHere("Accessing the physical memory size may require allocation and synchronization"); } - LOCK.lock(); - try { - if (!isInitialized()) { - if (isInitializing()) { - /* - * Recursive initializations need to wait for the one initializing thread to - * finish so as to get correct reads of the cachedSize value. - */ - try { - boolean expired = !CACHED_SIZE_AVAIL_LATCH.await(1L, TimeUnit.SECONDS); - if (expired) { - throw new InternalError("Expired latch!"); - } - VMError.guarantee(cachedSize != UNSET_SENTINEL, "Expected cached size to be set"); - return cachedSize; - } catch (InterruptedException e) { - throw VMError.shouldNotReachHere("Interrupt on countdown latch!"); + if (!isInitialized()) { + long memoryLimit = SubstrateOptions.MaxRAM.getValue(); + if (memoryLimit > 0) { + cachedSize = WordFactory.unsigned(memoryLimit); + } else { + LOCK.lock(); + try { + if (!isInitialized()) { + memoryLimit = Containers.memoryLimitInBytes(); + cachedSize = memoryLimit > 0 + ? WordFactory.unsigned(memoryLimit) + : ImageSingletons.lookup(PhysicalMemorySupport.class).size(); } + } finally { + LOCK.unlock(); } - 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/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java index 058880077c5c..fa2e5e45ba14 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java @@ -25,7 +25,6 @@ package com.oracle.svm.core.jdk; import java.util.Map; -import java.util.concurrent.atomic.AtomicInteger; import com.oracle.svm.core.NeverInline; import com.oracle.svm.core.SubstrateOptions; @@ -38,7 +37,6 @@ import com.oracle.svm.core.annotate.TargetClass; import com.oracle.svm.core.heap.PhysicalMemory; import com.oracle.svm.core.snippets.KnownIntrinsics; -import com.oracle.svm.core.util.VMError; import jdk.internal.misc.Unsafe; @@ -72,44 +70,26 @@ public static ClassLoader latestUserDefinedLoader0() { private static long directMemory; @Alias @InjectAccessors(PageAlignDirectMemoryAccessors.class) // private static boolean pageAlignDirectMemory; - - @Alias // - public static native void initLevel(int newVal); - - @Alias // - public static native int initLevel(); } final class DirectMemoryAccessors { + private static final long DIRECT_MEMORY_DURING_INITIALIZATION = 25 * 1024 * 1024; /* - * Full initialization is two-staged. First, we init directMemory to a static value (25MB) so - * that initialization of PhysicalMemory has a chance to finish. At that point isInintialized - * will be false, since we need to (potentially) set the value to the actual configured heap - * size. That can only be done once PhysicalMemory init completed. We'd introduce a cycle - * otherwise. + * Not volatile to avoid a memory barrier when reading the values. Instead, an explicit barrier + * is inserted when writing the values. */ - private static boolean isInitialized; - private static final int INITIALIZING = 1; - private static final int INITIALIZED = 2; - private static final AtomicInteger INIT_COUNT = new AtomicInteger(); - private static final long STATIC_DIRECT_MEMORY_AMOUNT = 25 * 1024 * 1024; + private static boolean initialized; private static long directMemory; static long getDirectMemory() { - if (!isInitialized) { - initialize(); + if (!initialized) { + return tryInitialize(); } return directMemory; } - private static void initialize() { - if (INIT_COUNT.get() == INITIALIZED) { - /* - * Safeguard for recursive init - */ - return; - } + private static long tryInitialize() { /* * The JDK method VM.saveAndRemoveProperties looks at the system property * "sun.nio.MaxDirectMemorySize". However, that property is always set by the Java HotSpot @@ -120,72 +100,41 @@ private static void initialize() { if (newDirectMemory == 0) { /* * No value explicitly specified. The default in the JDK in this case is the maximum - * heap size. However, we cannot rely on Runtime.maxMemory() until PhysicalMemory has - * fully initialized. Runtime.maxMemory() has a dependency on PhysicalMemory.size() - * which in turn depends on container support which might use NIO. To avoid this cycle, - * we first initialize the 'directMemory' field to an arbitrary value (25MB), and only - * use the Runtime.maxMemory() API once PhysicalMemory has fully initialized. + * heap size. */ - if (!PhysicalMemory.isInitialized()) { + if (PhysicalMemory.isInitializationInProgress()) { /* - * While initializing physical memory we might end up back here with an INIT_COUNT - * of 1, since we read the directMemory field during container support code - * execution which runs when PhysicalMemory is still initializing. + * When initializing PhysicalMemory, we use NIO/cgroups code that calls + * VM.getDirectMemory(). When this initialization is in progress, we need to prevent + * that Runtime.maxMemory() is called below because it would trigger a recursive + * initialization of PhysicalMemory. So, we return a temporary value. */ - VMError.guarantee(INIT_COUNT.get() <= INITIALIZING, "Initial run needs to have init count 0 or 1"); - newDirectMemory = STATIC_DIRECT_MEMORY_AMOUNT; // Static value during initialization - INIT_COUNT.setRelease(INITIALIZING); - } else { - VMError.guarantee(INIT_COUNT.get() <= INITIALIZING, "Runtime.maxMemory() invariant"); - /* - * Once we know PhysicalMemory has been properly initialized we can use - * Runtime.maxMemory(). Note that we might end up in this branch for code explicitly - * using the JDK cgroups code. At that point PhysicalMemory has likely been - * initialized. - */ - INIT_COUNT.setRelease(INITIALIZED); - newDirectMemory = Runtime.getRuntime().maxMemory(); - } - } else { - /* - * For explicitly set direct memory we are done - */ - Unsafe.getUnsafe().storeFence(); - directMemory = newDirectMemory; - isInitialized = true; - if (Target_jdk_internal_misc_VM.initLevel() < 1) { - // only the first accessor needs to set this - Target_jdk_internal_misc_VM.initLevel(1); + return DIRECT_MEMORY_DURING_INITIALIZATION; } - return; + newDirectMemory = Runtime.getRuntime().maxMemory(); } - VMError.guarantee(newDirectMemory > 0, "New direct memory should be initialized"); - Unsafe.getUnsafe().storeFence(); + /* + * The initialization is not synchronized, so multiple threads can race. Usually this will + * lead to the same value, unless the runtime options are modified concurrently - which is + * possible but not a case we care about. + */ directMemory = newDirectMemory; - if (PhysicalMemory.isInitialized() && INITIALIZED == INIT_COUNT.get()) { - /* - * Complete initialization hand-shake once PhysicalMemory is properly initialized. Also - * set the VM init level to 1 so as to provoke the NIO code to re-set the internal - * MAX_MEMORY field. - */ - isInitialized = true; - if (Target_jdk_internal_misc_VM.initLevel() < 1) { - // only the first accessor needs to set this - Target_jdk_internal_misc_VM.initLevel(1); - } - } + + /* Ensure values are published to other threads before marking fields as initialized. */ + Unsafe.getUnsafe().storeFence(); + initialized = true; + + return newDirectMemory; } } final class PageAlignDirectMemoryAccessors { - /* * Not volatile to avoid a memory barrier when reading the values. Instead, an explicit barrier * is inserted when writing the values. */ private static boolean initialized; - private static boolean pageAlignDirectMemory; static boolean getPageAlignDirectMemory() { From a60694004fd8e71c0af98fed8bbd25a066285d18 Mon Sep 17 00:00:00 2001 From: Christian Haeubl Date: Fri, 6 Oct 2023 14:04:39 +0200 Subject: [PATCH 10/11] Prevent that an invalid direct memory size is cached in java.nio.Bits#MAX_MEMORY. --- .../svm/core/jdk/Target_java_nio_Bits.java | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_java_nio_Bits.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_java_nio_Bits.java index 292659db7ded..24d193ba1be0 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_java_nio_Bits.java +++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_java_nio_Bits.java @@ -27,9 +27,11 @@ import java.util.concurrent.atomic.AtomicLong; import com.oracle.svm.core.annotate.Alias; +import com.oracle.svm.core.annotate.InjectAccessors; import com.oracle.svm.core.annotate.RecomputeFieldValue; import com.oracle.svm.core.annotate.RecomputeFieldValue.Kind; import com.oracle.svm.core.annotate.TargetClass; +import com.oracle.svm.core.heap.PhysicalMemory; @TargetClass(className = "java.nio.Bits") final class Target_java_nio_Bits { @@ -40,9 +42,9 @@ final class Target_java_nio_Bits { private static int PAGE_SIZE = -1; @Alias @RecomputeFieldValue(kind = Kind.FromAlias) // - private static boolean MEMORY_LIMIT_SET = false; - @Alias @RecomputeFieldValue(kind = Kind.FromAlias) // - private static long MAX_MEMORY = -1; + private static boolean MEMORY_LIMIT_SET = true; + @Alias @InjectAccessors(MaxMemoryAccessor.class) // + private static long MAX_MEMORY; @Alias @RecomputeFieldValue(kind = Kind.FromAlias) // private static AtomicLong RESERVED_MEMORY = new AtomicLong(); @@ -53,3 +55,23 @@ final class Target_java_nio_Bits { // Checkstyle: resume } + +/** + * {@code java.nio.Bits} caches the max. direct memory size in the field {@code MAX_MEMORY}. We + * disable this cache and always call {@link DirectMemoryAccessors#getDirectMemory()} instead, which + * uses our own cache. Otherwise, it could happen that {@code MAX_MEMORY} caches a temporary value + * that is used during early VM startup, before {@link PhysicalMemory} is fully initialized. + */ +final class MaxMemoryAccessor { + // Checkstyle: stop + + static long getMAX_MEMORY() { + return DirectMemoryAccessors.getDirectMemory(); + } + + static void setMAX_MEMORY(long value) { + /* Nothing to do. */ + } + + // Checkstyle: resume +} From baeb876480aa47c56b043a4d939927d3d770e9c6 Mon Sep 17 00:00:00 2001 From: ol-automation_ww Date: Sat, 7 Oct 2023 19:51:29 +0000 Subject: [PATCH 11/11] [GR-23997] Periodic update of the graal import (2023-10-06). PullRequest: js/2938 --- vm/mx.vm/suite.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vm/mx.vm/suite.py b/vm/mx.vm/suite.py index 38494e0fc6eb..1ff0d5d3ba12 100644 --- a/vm/mx.vm/suite.py +++ b/vm/mx.vm/suite.py @@ -33,7 +33,7 @@ "name": "graal-nodejs", "subdir": True, "dynamic": True, - "version": "1622bc1ecd11eb5ea5d9173702e3f68033d5ac1d", + "version": "6dd82d9f89f1be47d34b82f16d99afffd2a309af", "urls" : [ {"url" : "https://github.com/graalvm/graaljs.git", "kind" : "git"}, ] @@ -42,7 +42,7 @@ "name": "graal-js", "subdir": True, "dynamic": True, - "version": "1622bc1ecd11eb5ea5d9173702e3f68033d5ac1d", + "version": "6dd82d9f89f1be47d34b82f16d99afffd2a309af", "urls": [ {"url": "https://github.com/graalvm/graaljs.git", "kind" : "git"}, ]