[GR-30807] Preparations to use a dedicated reference handler thread by default.

PullRequest: graal/10087

Author: christianhaeubl

sdk/src/org.graalvm.nativeimage/snapshot.sigtest

@@ -164,21 +164,23 @@ CLSS public final static org.graalvm.nativeimage.Isolates$CreateIsolateParameter
  outer org.graalvm.nativeimage.Isolates
 innr public final static Builder
 meth public java.lang.String getAuxiliaryImagePath()
+meth public java.util.List<java.lang.String> getArguments()
 meth public org.graalvm.word.UnsignedWord getAuxiliaryImageReservedSpaceSize()
 meth public org.graalvm.word.UnsignedWord getReservedAddressSpaceSize()
 meth public static org.graalvm.nativeimage.Isolates$CreateIsolateParameters getDefault()
 supr java.lang.Object
-hfds DEFAULT,auxiliaryImagePath,auxiliaryImageReservedSpaceSize,reservedAddressSpaceSize
+hfds DEFAULT,arguments,auxiliaryImagePath,auxiliaryImageReservedSpaceSize,reservedAddressSpaceSize
 
 CLSS public final static org.graalvm.nativeimage.Isolates$CreateIsolateParameters$Builder
  outer org.graalvm.nativeimage.Isolates$CreateIsolateParameters
 cons public init()
 meth public org.graalvm.nativeimage.Isolates$CreateIsolateParameters build()
+meth public org.graalvm.nativeimage.Isolates$CreateIsolateParameters$Builder appendArgument(java.lang.String)
 meth public org.graalvm.nativeimage.Isolates$CreateIsolateParameters$Builder auxiliaryImagePath(java.lang.String)
 meth public org.graalvm.nativeimage.Isolates$CreateIsolateParameters$Builder auxiliaryImageReservedSpaceSize(org.graalvm.word.UnsignedWord)
 meth public org.graalvm.nativeimage.Isolates$CreateIsolateParameters$Builder reservedAddressSpaceSize(org.graalvm.word.UnsignedWord)
 supr java.lang.Object
-hfds auxiliaryImagePath,auxiliaryImageReservedSpaceSize,reservedAddressSpaceSize
+hfds arguments,auxiliaryImagePath,auxiliaryImageReservedSpaceSize,reservedAddressSpaceSize
 
 CLSS public final static org.graalvm.nativeimage.Isolates$IsolateException
  outer org.graalvm.nativeimage.Isolates
@@ -475,7 +477,7 @@ innr public final static FatalExceptionHandler
 innr public final static NotIncludedAutomatically
 intf java.lang.annotation.Annotation
 meth public abstract !hasdefault java.lang.Class<? extends java.util.function.BooleanSupplier> include()
-meth public abstract !hasdefault java.lang.Class<? extends org.graalvm.nativeimage.c.function.CEntryPoint$ExceptionHandler> exceptionHandler()
+meth public abstract !hasdefault java.lang.Class<?> exceptionHandler()
 meth public abstract !hasdefault java.lang.String name()
 meth public abstract !hasdefault java.lang.String[] documentation()
 meth public abstract !hasdefault org.graalvm.nativeimage.c.function.CEntryPoint$Builtin builtin()

sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/Isolates.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * The Universal Permissive License (UPL), Version 1.0
@@ -40,6 +40,9 @@
  */
 package org.graalvm.nativeimage;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 import java.util.Objects;
 
 import org.graalvm.nativeimage.impl.IsolateSupport;
@@ -90,13 +93,15 @@ public static final class Builder {
             private UnsignedWord reservedAddressSpaceSize;
             private String auxiliaryImagePath;
             private UnsignedWord auxiliaryImageReservedSpaceSize;
+            private final List<String> arguments;
 
             /**
              * Creates a new builder with default values.
              *
              * @since 19.0
              */
             public Builder() {
+                arguments = new ArrayList<>();
             }
 
             /**
@@ -131,14 +136,27 @@ public Builder auxiliaryImageReservedSpaceSize(UnsignedWord size) {
                 return this;
             }
 
+            /**
+             * Appends an isolate argument. The syntax for arguments is the same as the one that is
+             * used on the command-line when starting Native Image (e.g., {@code
+             * -XX:+UseReferenceHandlerThread}). If the same argument is added multiple times, the
+             * last specified value will be used.
+             *
+             * @since 22.1
+             */
+            public Builder appendArgument(String argument) {
+                this.arguments.add(argument);
+                return this;
+            }
+
             /**
              * Produces the final {@link CreateIsolateParameters} with the values set previously by
              * the builder methods.
              *
              * @since 19.0
              */
             public CreateIsolateParameters build() {
-                return new CreateIsolateParameters(reservedAddressSpaceSize, auxiliaryImagePath, auxiliaryImageReservedSpaceSize);
+                return new CreateIsolateParameters(reservedAddressSpaceSize, auxiliaryImagePath, auxiliaryImageReservedSpaceSize, arguments);
             }
         }
 
@@ -156,11 +174,13 @@ public static CreateIsolateParameters getDefault() {
         private final UnsignedWord reservedAddressSpaceSize;
         private final String auxiliaryImagePath;
         private final UnsignedWord auxiliaryImageReservedSpaceSize;
+        private final List<String> arguments;
 
-        private CreateIsolateParameters(UnsignedWord reservedAddressSpaceSize, String auxiliaryImagePath, UnsignedWord auxiliaryImageReservedSpaceSize) {
+        private CreateIsolateParameters(UnsignedWord reservedAddressSpaceSize, String auxiliaryImagePath, UnsignedWord auxiliaryImageReservedSpaceSize, List<String> arguments) {
             this.reservedAddressSpaceSize = reservedAddressSpaceSize;
             this.auxiliaryImagePath = auxiliaryImagePath;
             this.auxiliaryImageReservedSpaceSize = auxiliaryImageReservedSpaceSize;
+            this.arguments = arguments;
         }
 
         /**
@@ -191,6 +211,15 @@ public String getAuxiliaryImagePath() {
         public UnsignedWord getAuxiliaryImageReservedSpaceSize() {
             return auxiliaryImageReservedSpaceSize;
         }
+
+        /**
+         * Returns the list of additional isolate arguments.
+         *
+         * @since 22.1
+         */
+        public List<String> getArguments() {
+            return Collections.unmodifiableList(arguments);
+        }
     }
 
     /**

substratevm/mx.substratevm/mx_substratevm.py

@@ -995,6 +995,9 @@ def _native_image_launcher_extra_jvm_args():
                 # These 2 arguments provide walkable call stacks for a crash in libgraal
                 '-H:+PreserveFramePointer',
                 '-H:-DeleteLocalSymbols',
+
+                # No VM-internal threads may be spawned for libgraal.
+                '-H:-AllowVMInternalThreads',
             ],
         ),
     ],

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/GCImpl.java

@@ -81,7 +81,6 @@
 import com.oracle.svm.core.stack.JavaStackWalk;
 import com.oracle.svm.core.stack.JavaStackWalker;
 import com.oracle.svm.core.stack.ThreadStackPrinter;
-import com.oracle.svm.core.thread.JavaThreads;
 import com.oracle.svm.core.thread.JavaVMOperation;
 import com.oracle.svm.core.thread.NativeVMOperation;
 import com.oracle.svm.core.thread.NativeVMOperationData;
@@ -148,12 +147,11 @@ public void maybeCauseUserRequestedCollection() {
 
     private void collect(GCCause cause, boolean forceFullGC) {
         if (!hasNeverCollectPolicy()) {
-            UnsignedWord requestingEpoch = possibleCollectionPrologue();
             boolean outOfMemory = collectWithoutAllocating(cause, forceFullGC);
             if (outOfMemory) {
                 throw OUT_OF_MEMORY_ERROR;
             }
-            possibleCollectionEpilogue(requestingEpoch);
+            doReferenceHandling();
         }
     }
 
@@ -1057,50 +1055,53 @@ private void finishCollection() {
         collectionInProgress = false;
     }
 
-    UnsignedWord possibleCollectionPrologue() {
-        return getCollectionEpoch();
-    }
-
     /**
-     * Do whatever is necessary if a collection occurred since the call to
-     * {@link #possibleCollectionPrologue()}. Note that this method may get called by several
-     * threads for the same collection.
+     * NOTE: All code that is transitively reachable from this method may get executed as a side
+     * effect of a GC or as a side effect of an allocation slow path. To prevent hard to debug
+     * transient issues, we execute as little code as possible in this method. Multiple threads may
+     * execute this method concurrently.
+     *
+     * Without a dedicated reference handler thread, arbitrary complex code can get executed as a
+     * side effect of this method. So, allocations of Java objects or an explicitly triggered GC can
+     * result in a recursive invocation of methods.
+     *
+     * This can have the effect that global state changes unexpectedly and may result in issues that
+     * look similar to races but that can even happen in single-threaded environments, e.g.:
+     *
+     * <pre>
+     * {@code
+     * private static Object singleton;
+     *
+     * private static synchronized Object createSingleton() {
+     *     if (singleton == null) {
+     *         Object o = new Object();
+     *         // If the allocation above enters the allocation slow path code, then it is possible
+     *         // that doReferenceHandling() gets executed by the current thread. If the method
+     *         // createSingleton() is called as a side effect of doReferenceHandling(), then the
+     *         // assertion below may fail because the singleton got already initialized by the same
+     *         // thread in the meanwhile.
+     *         assert singleton == null;
+     *         singleton = o;
+     *     }
+     *     return result;
+     * }
+     * }
+     * </pre>
      */
-    void possibleCollectionEpilogue(UnsignedWord requestingEpoch) {
-        if (requestingEpoch.aboveOrEqual(getCollectionEpoch())) {
-            /* No GC happened, so do not run any epilogue. */
-            return;
-
-        } else if (VMOperation.isInProgress()) {
-            /*
-             * We are inside a VMOperation where we are not allowed to do certain things, e.g.,
-             * perform a synchronization (because it can deadlock when a lock is held outside the
-             * VMOperation).
-             *
-             * Note that the GC operation we are running the epilogue for is no longer in progress,
-             * otherwise this check would always return.
-             */
-            return;
-
-        } else if (!JavaThreads.currentJavaThreadInitialized()) {
-            /*
-             * Too early in the attach sequence of a thread to do anything useful, e.g., perform a
-             * synchronization. Probably the allocation slow path for the first allocation of that
-             * thread caused this epilogue.
-             */
+    static void doReferenceHandling() {
+        if (ReferenceHandler.useDedicatedThread()) {
             return;
         }
 
-        Timer refsTimer = new Timer("Enqueuing pending references and invoking internal cleaners");
-        Timer timer = refsTimer.open();
-        try {
-            ReferenceHandler.maybeProcessCurrentlyPending();
-        } finally {
-            timer.close();
-        }
-        if (SubstrateGCOptions.VerboseGC.getValue() && HeapOptions.PrintGCTimes.getValue()) {
-            Timers.logOneTimer(Log.log(), "[GC epilogue reference processing: ", refsTimer);
-            Log.log().string("]");
+        /* Most of the time, we won't have a pending reference list. So, we do that check first. */
+        if (HeapImpl.getHeapImpl().hasReferencePendingListUnsafe() && ReferenceHandler.isReferenceHandlingAllowed()) {
+            long startTime = System.nanoTime();
+            ReferenceHandler.processPendingReferencesInRegularThread();
+
+            if (SubstrateGCOptions.VerboseGC.getValue() && HeapOptions.PrintGCTimes.getValue()) {
+                long executionTime = System.nanoTime() - startTime;
+                Log.log().string("[GC epilogue reference processing and cleaners: ").signed(executionTime).string("]").newline();
+            }
         }
     }
 

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/HeapImpl.java

@@ -34,7 +34,6 @@
 import org.graalvm.compiler.nodes.gc.BarrierSet;
 import org.graalvm.compiler.word.Word;
 import org.graalvm.nativeimage.CurrentIsolate;
-import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.IsolateThread;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
@@ -64,7 +63,7 @@
 import com.oracle.svm.core.heap.ObjectHeader;
 import com.oracle.svm.core.heap.ObjectVisitor;
 import com.oracle.svm.core.heap.PhysicalMemory;
-import com.oracle.svm.core.heap.ReferenceHandlerThreadSupport;
+import com.oracle.svm.core.heap.ReferenceHandlerThread;
 import com.oracle.svm.core.heap.ReferenceInternals;
 import com.oracle.svm.core.heap.RuntimeCodeInfoGCSupport;
 import com.oracle.svm.core.hub.DynamicHub;
@@ -474,7 +473,7 @@ public BarrierSet createBarrierSet(MetaAccessProvider metaAccess) {
 
     @SuppressFBWarnings(value = "VO_VOLATILE_INCREMENT", justification = "Only the GC increments the volatile field 'refListOfferCounter'.")
     void addToReferencePendingList(Reference<?> list) {
-        VMOperation.guaranteeGCInProgress("Must only be called during a GC.");
+        assert VMOperation.isGCInProgress();
         if (list == null) {
             return;
         }
@@ -505,12 +504,17 @@ void addToReferencePendingList(Reference<?> list) {
     public boolean hasReferencePendingList() {
         REF_MUTEX.lockNoTransition();
         try {
-            return refPendingList != null;
+            return hasReferencePendingListUnsafe();
         } finally {
             REF_MUTEX.unlock();
         }
     }
 
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
+    boolean hasReferencePendingListUnsafe() {
+        return refPendingList != null;
+    }
+
     @Override
     public void waitForReferencePendingList() throws InterruptedException {
         /*
@@ -533,7 +537,7 @@ public void waitForReferencePendingList() throws InterruptedException {
          * everything in the reverse order here: we read the wakeup count *before* the call to
          * Thread.interrupted().
          */
-        assert Thread.currentThread() == ImageSingletons.lookup(ReferenceHandlerThreadSupport.class).getThread();
+        assert ReferenceHandlerThread.isReferenceHandlerThread();
         long initialOffers = refListOfferCounter;
         long initialWakeUps = refListWaiterWakeUpCounter;
         if (hasReferencePendingList()) {

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/ThreadLocalAllocation.java

@@ -152,26 +152,25 @@ private static Object slowPathNewInstance(Word objectHeader, UnsignedWord size)
         StackOverflowCheck.singleton().makeYellowZoneAvailable();
         try {
             DynamicHub hub = ObjectHeaderImpl.getObjectHeaderImpl().dynamicHubFromObjectHeader(objectHeader);
-            UnsignedWord gcEpoch = HeapImpl.getHeapImpl().getGCImpl().possibleCollectionPrologue();
 
             // the instance either is a frame instance or the size can be read from the hub
-            if (!hub.isStoredContinuationClass()) {
-                assert size.equal(hub.getLayoutEncoding());
-            }
+            assert hub.isStoredContinuationClass() || size.equal(hub.getLayoutEncoding());
             Object result = slowPathNewInstanceWithoutAllocating(hub, size);
-            /*
-             * If a collection happened, do follow-up tasks now that allocation, etc., is allowed.
-             */
-            HeapImpl.getHeapImpl().getGCImpl().possibleCollectionEpilogue(gcEpoch);
             runSlowPathHooks();
             return result;
         } finally {
             StackOverflowCheck.singleton().protectYellowZone();
         }
     }
 
-    /** Use the end of slow-path allocation as a place to run periodic hook code. */
+    /**
+     * NOTE: All code that is transitively reachable from this method may get executed as a side
+     * effect of an allocation slow path. To prevent hard to debug transient issues, we execute as
+     * little code as possible in this method (see {@link GCImpl#doReferenceHandling()} for more
+     * details). Multiple threads may execute this method concurrently.
+     */
     private static void runSlowPathHooks() {
+        GCImpl.doReferenceHandling();
         GCImpl.getPolicy().updateSizeParameters();
     }
 
@@ -213,12 +212,7 @@ private static Object slowPathNewArray(Word objectHeader, int length, int fillSt
                 throw new OutOfMemoryError("Array allocation too large.");
             }
 
-            UnsignedWord gcEpoch = HeapImpl.getHeapImpl().getGCImpl().possibleCollectionPrologue();
             Object result = slowPathNewArrayWithoutAllocating(hub, length, size, fillStartOffset);
-            /*
-             * If a collection happened, do follow-up tasks now that allocation, etc., is allowed.
-             */
-            HeapImpl.getHeapImpl().getGCImpl().possibleCollectionEpilogue(gcEpoch);
             runSlowPathHooks();
             return result;
         } finally {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/Containers.java

@@ -26,6 +26,7 @@
 
 import static com.oracle.svm.core.Containers.Options.PreferContainerQuotaForCPUCount;
 import static com.oracle.svm.core.Containers.Options.UseContainerSupport;
+import static com.oracle.svm.core.option.RuntimeOptionKey.RuntimeOptionKeyFlag.RelevantForCompilationIsolates;
 
 import org.graalvm.compiler.options.Option;
 import org.graalvm.compiler.serviceprovider.JavaVersionUtil;
@@ -54,7 +55,7 @@ public static class Options {
 
         @Option(help = "Calculate the container CPU availability based on the value of quotas (if set), when true. " +
                         "Otherwise, use the CPU shares value, provided it is less than quota.")//
-        public static final RuntimeOptionKey<Boolean> PreferContainerQuotaForCPUCount = new RuntimeOptionKey<>(true);
+        public static final RuntimeOptionKey<Boolean> PreferContainerQuotaForCPUCount = new RuntimeOptionKey<>(true, RelevantForCompilationIsolates);
     }
 
     /** Sentinel used when the value is unknown. */

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/IsolateArgumentParser.java

@@ -55,7 +55,8 @@
  * isolate is fully started.
  */
 public class IsolateArgumentParser {
-    private static final RuntimeOptionKey<?>[] OPTIONS = {SubstrateGCOptions.MinHeapSize, SubstrateGCOptions.MaxHeapSize, SubstrateGCOptions.MaxNewSize};
+    private static final RuntimeOptionKey<?>[] OPTIONS = {SubstrateGCOptions.MinHeapSize, SubstrateGCOptions.MaxHeapSize, SubstrateGCOptions.MaxNewSize,
+                    SubstrateOptions.ConcealedOptions.UseReferenceHandlerThread};
     private static final int OPTION_COUNT = OPTIONS.length;
     private static final CGlobalData<CCharPointer> OPTION_NAMES = CGlobalDataFactory.createBytes(IsolateArgumentParser::createOptionNames);
     private static final CGlobalData<CIntPointer> OPTION_NAME_POSITIONS = CGlobalDataFactory.createBytes(IsolateArgumentParser::createOptionNamePosition);
@@ -185,11 +186,15 @@ public void persistOptions(CLongPointer parsedArgs) {
 
     public void verifyOptionValues() {
         for (int i = 0; i < OPTION_COUNT; i++) {
-            validate(OPTIONS[i], getParsedOptionValue(i));
+            validate(OPTIONS[i], getOptionValue(i));
         }
     }
 
-    private static Object getParsedOptionValue(int index) {
+    public static boolean getBooleanOptionValue(int index) {
+        return PARSED_OPTION_VALUES[index] == 1;
+    }
+
+    private static Object getOptionValue(int index) {
         Class<?> optionValueType = OPTIONS[index].getDescriptor().getOptionValueType();
         long value = PARSED_OPTION_VALUES[index];
         if (optionValueType == Boolean.class) {