Do not use secondary monitor map for locking on Class

Author: Christian Wimmer

substratevm/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 This changelog summarizes major changes to GraalVM Native Image.
 
+## Version 22.1.0
+* (GR-35898) Improved handling of static synchronized methods: the lock is no longer stored in the secondary monitor map, but in the mutable DynamicHubCompanion object.
+
 ## Version 22.0.0
 * (GR-33930) Decouple HostedOptionParser setup from classpath/modulepath scanning (use ServiceLoader for collecting options).
 * (GR-33504) Implement --add-reads for native-image and fix --add-opens error handling.

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHub.java

@@ -88,9 +88,6 @@
 public final class DynamicHub implements JavaKind.FormatWithToString, AnnotatedElement, java.lang.reflect.Type, GenericDeclaration, Serializable,
                 Target_java_lang_invoke_TypeDescriptor_OfField, Target_java_lang_constant_Constable {
 
-    /** Marker value for {@link #classLoader}. */
-    static final Object NO_CLASS_LOADER = new Object();
-
     @Substitute //
     private static final Class<?>[] EMPTY_CLASS_ARRAY = new Class<?>[0];
 
@@ -272,11 +269,6 @@ public final class DynamicHub implements JavaKind.FormatWithToString, AnnotatedE
      */
     private ClassInitializationInfo classInitializationInfo;
 
-    /**
-     * Classloader used for loading this class during image-build time.
-     */
-    private final Object classLoader;
-
     /**
      * Array containing this type's type check id information. During a type check, a requested
      * column of this array is read to determine if this value fits within the range of ids which
@@ -305,7 +297,7 @@ public void setModule(Module module) {
         this.module = module;
     }
 
-    private final LazyFinalReference<DynamicHubCompanion> companion = new LazyFinalReference<>(() -> new DynamicHubCompanion(this));
+    private final DynamicHubCompanion companion;
 
     @Platforms(Platform.HOSTED_ONLY.class)
     public DynamicHub(Class<?> hostedJavaClass, String name, HubType hubType, ReferenceType referenceType, Object isLocalClass, Object isAnonymousClass, DynamicHub superType, DynamicHub componentHub,
@@ -321,7 +313,6 @@ public DynamicHub(Class<?> hostedJavaClass, String name, HubType hubType, Refere
         this.componentType = componentHub;
         this.sourceFileName = sourceFileName;
         this.modifiers = modifiers;
-        this.classLoader = PredefinedClassesSupport.isPredefined(hostedJavaClass) ? NO_CLASS_LOADER : classLoader;
         this.nestHost = nestHost;
 
         this.flags = NumUtil.safeToUByte(makeFlag(IS_PRIMITIVE_FLAG_BIT, hostedJavaClass.isPrimitive()) |
@@ -332,6 +323,8 @@ public DynamicHub(Class<?> hostedJavaClass, String name, HubType hubType, Refere
                         makeFlag(HAS_DEFAULT_METHODS_FLAG_BIT, hasDefaultMethods) |
                         makeFlag(DECLARES_DEFAULT_METHODS_FLAG_BIT, declaresDefaultMethods) |
                         makeFlag(IS_SEALED_FLAG_BIT, isSealed));
+
+        this.companion = new DynamicHubCompanion(hostedJavaClass, classLoader);
     }
 
     @Platforms(Platform.HOSTED_ONLY.class)
@@ -560,6 +553,10 @@ public static DynamicHub fromClass(Class<?> clazz) {
         return SubstrateUtil.cast(clazz, DynamicHub.class);
     }
 
+    public DynamicHubCompanion getCompanion() {
+        return companion;
+    }
+
     /*
      * Note that this method must be a static method and not an instance method, otherwise null
      * values cannot be converted.
@@ -696,18 +693,15 @@ public InputStream getResourceAsStream(String resourceName) {
 
     @Substitute
     private ClassLoader getClassLoader0() {
-        if (classLoader == NO_CLASS_LOADER) {
-            return companion.get().getClassLoader();
-        }
-        return (ClassLoader) classLoader;
+        return companion.getClassLoader();
     }
 
     public boolean isLoaded() {
-        return classLoader != NO_CLASS_LOADER || (companion.isPresent() && companion.get().hasClassLoader());
+        return companion.hasClassLoader();
     }
 
     void setClassLoaderAtRuntime(ClassLoader loader) {
-        companion.get().setClassLoader(loader);
+        companion.setClassLoader(loader);
     }
 
     @Substitute
@@ -1066,7 +1060,7 @@ private Method getMethod(@SuppressWarnings("hiding") String name, Class<?>... pa
          * The original code of getMethods() does a recursive search to avoid creating objects for
          * all public methods. We prepare them during the image build and can just iterate here.
          */
-        Method method = searchMethods(companion.get().getCompleteReflectionData().publicMethods, name, parameterTypes);
+        Method method = searchMethods(companion.getCompleteReflectionData(this).publicMethods, name, parameterTypes);
         if (method == null) {
             throw new NoSuchMethodException(describeMethod(getName() + "." + name + "(", parameterTypes, ")"));
         }
@@ -1112,7 +1106,8 @@ private Class<?>[] getClasses() {
 
     @Substitute
     private Constructor<?>[] privateGetDeclaredConstructors(boolean publicOnly) {
-        return publicOnly ? companion.get().getCompleteReflectionData().publicConstructors : companion.get().getCompleteReflectionData().declaredConstructors;
+        ReflectionData reflectionData = companion.getCompleteReflectionData(this);
+        return publicOnly ? reflectionData.publicConstructors : reflectionData.declaredConstructors;
     }
 
     @Substitute
@@ -1122,7 +1117,8 @@ private Field[] privateGetDeclaredFields(boolean publicOnly) {
 
     @Substitute
     private Method[] privateGetDeclaredMethods(boolean publicOnly) {
-        return publicOnly ? companion.get().getCompleteReflectionData().declaredPublicMethods : companion.get().getCompleteReflectionData().declaredMethods;
+        ReflectionData reflectionData = companion.getCompleteReflectionData(this);
+        return publicOnly ? reflectionData.declaredPublicMethods : reflectionData.declaredMethods;
     }
 
     @Substitute
@@ -1132,7 +1128,7 @@ private Field[] privateGetPublicFields() {
 
     @Substitute
     Method[] privateGetPublicMethods() {
-        return companion.get().getCompleteReflectionData().publicMethods;
+        return companion.getCompleteReflectionData(this).publicMethods;
     }
 
     @KeepOriginal
@@ -1292,7 +1288,7 @@ public String getPackageName() {
         if (SubstrateUtil.HOSTED) { // avoid eager initialization in image heap
             return computePackageName();
         }
-        return companion.get().getPackageName();
+        return companion.getPackageName(this);
     }
 
     String computePackageName() {
@@ -1330,7 +1326,7 @@ public Object[] getSigners() {
 
     @Substitute
     public ProtectionDomain getProtectionDomain() {
-        return companion.get().getProtectionDomain();
+        return companion.getProtectionDomain();
     }
 
     @TargetElement(onlyWith = JDK17OrLater.class)
@@ -1340,7 +1336,7 @@ private ProtectionDomain protectionDomain() {
     }
 
     void setProtectionDomainAtRuntime(ProtectionDomain protectionDomain) {
-        companion.get().setProtectionDomain(protectionDomain);
+        companion.setProtectionDomain(protectionDomain);
     }
 
     @Substitute

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHubCompanion.java

@@ -26,8 +26,6 @@
 
 // Checkstyle: allow reflection
 
-import static com.oracle.svm.core.hub.DynamicHub.NO_CLASS_LOADER;
-
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Executable;
 import java.lang.reflect.Method;
@@ -39,27 +37,38 @@
 
 import org.graalvm.collections.Pair;
 import org.graalvm.nativeimage.ImageSingletons;
+import org.graalvm.nativeimage.Platform;
+import org.graalvm.nativeimage.Platforms;
 
 import com.oracle.svm.core.hub.DynamicHub.ReflectionData;
 import com.oracle.svm.core.jdk.ProtectionDomainSupport;
 import com.oracle.svm.core.reflect.MethodMetadataDecoder;
 import com.oracle.svm.core.reflect.MethodMetadataDecoder.MethodDescriptor;
 import com.oracle.svm.core.util.VMError;
 
-/** An optional, non-immutable companion to a {@link DynamicHub} instance. */
+/**
+ * The mutable parts of a {@link DynamicHub} instance.
+ */
 public final class DynamicHubCompanion {
-    private final DynamicHub hub;
+    /** Marker value for {@link #classLoader}. */
+    private static final Object NO_CLASS_LOADER = new Object();
 
     private String packageName;
-    private Object classLoader = NO_CLASS_LOADER;
+    /**
+     * Classloader used for loading this class. Most classes have the correct class loader set
+     * already at image build time. {@link PredefinedClassesSupport Predefined classes} get their
+     * classloader only at run time, before "loading" the field value is {@link #NO_CLASS_LOADER}.
+     */
+    private Object classLoader;
     private ProtectionDomain protectionDomain;
     private ReflectionData completeReflectionData;
 
-    public DynamicHubCompanion(DynamicHub hub) {
-        this.hub = hub;
+    @Platforms(Platform.HOSTED_ONLY.class)
+    DynamicHubCompanion(Class<?> hostedJavaClass, ClassLoader classLoader) {
+        this.classLoader = PredefinedClassesSupport.isPredefined(hostedJavaClass) ? NO_CLASS_LOADER : classLoader;
     }
 
-    public String getPackageName() {
+    String getPackageName(DynamicHub hub) {
         if (packageName == null) {
             packageName = hub.computePackageName();
         }
@@ -70,30 +79,30 @@ boolean hasClassLoader() {
         return classLoader != NO_CLASS_LOADER;
     }
 
-    public ClassLoader getClassLoader() {
+    ClassLoader getClassLoader() {
         Object loader = classLoader;
         VMError.guarantee(loader != NO_CLASS_LOADER);
         return (ClassLoader) loader;
     }
 
-    public void setClassLoader(ClassLoader loader) {
+    void setClassLoader(ClassLoader loader) {
         VMError.guarantee(classLoader == NO_CLASS_LOADER && loader != NO_CLASS_LOADER);
         classLoader = loader;
     }
 
-    public ProtectionDomain getProtectionDomain() {
+    ProtectionDomain getProtectionDomain() {
         if (protectionDomain == null) {
             protectionDomain = ProtectionDomainSupport.allPermDomain();
         }
         return protectionDomain;
     }
 
-    public void setProtectionDomain(ProtectionDomain domain) {
+    void setProtectionDomain(ProtectionDomain domain) {
         VMError.guarantee(protectionDomain == null && domain != null);
         protectionDomain = domain;
     }
 
-    public ReflectionData getCompleteReflectionData() {
+    ReflectionData getCompleteReflectionData(DynamicHub hub) {
         if (completeReflectionData == null) {
             List<Method> newDeclaredMethods = new ArrayList<>(Arrays.asList(hub.rd.declaredMethods));
             List<Method> newPublicMethods = new ArrayList<>(Arrays.asList(hub.rd.publicMethods));

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/RandomAccessors.java

@@ -59,13 +59,6 @@ protected AtomicLong getOrInitializeSeeder() {
     }
 
     // Checkstyle: allow synchronization
-    /**
-     * It is important that this synchronization is on an instance method and not on a static
-     * method. A static synchronized method will lock the java.lang.Class object and SVM currently
-     * uses a secondary storage map for locking on classes. Syncronizing on a class object can lead
-     * to recursive locking problems when this particular code is called from the constructor of
-     * JavaVMOperation.
-     */
     private synchronized AtomicLong initialize() {
         AtomicLong result = seeder;
         if (result != null) {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java

@@ -57,6 +57,7 @@
 import com.oracle.svm.core.annotate.TargetClass;
 import com.oracle.svm.core.annotate.Uninterruptible;
 import com.oracle.svm.core.hub.DynamicHub;
+import com.oracle.svm.core.hub.DynamicHubCompanion;
 import com.oracle.svm.core.snippets.SubstrateForeignCallTarget;
 import com.oracle.svm.core.stack.StackOverflowCheck;
 import com.oracle.svm.core.thread.JavaContinuations;
@@ -174,6 +175,12 @@ protected static void onMonitorUnlocked() {
                 monitorTypes.add(Class.forName("jdk.internal.ref.PhantomCleanable"));
             }
 
+            /*
+             * Use as the delegate for locking on {@link Class} (i.e. {@link DynamicHub}) since the
+             * hub itself must be immutable.
+             */
+            monitorTypes.add(DynamicHubCompanion.class);
+
             FORCE_MONITOR_SLOT_TYPES = Collections.unmodifiableSet(monitorTypes);
         } catch (ClassNotFoundException e) {
             throw VMError.shouldNotReachHere("Error building the list of types that always need a monitor slot.", e);
@@ -422,38 +429,20 @@ protected static int getMonitorOffset(Object obj) {
         return DynamicHub.fromClass(obj.getClass()).getMonitorOffset();
     }
 
-    private static final Object CLEANER_CLASS;
-    private static final Object CLEANER_REPLACEMENT;
-
-    static {
-        try {
-            CLEANER_CLASS = Class.forName("jdk.internal.ref.Cleaner");
-        } catch (ClassNotFoundException ex) {
-            throw VMError.shouldNotReachHere(ex);
-        }
-        CLEANER_REPLACEMENT = new Object();
-        VMError.guarantee(FORCE_MONITOR_SLOT_TYPES.contains(CLEANER_REPLACEMENT.getClass()), "Must have a monitor slot for Cleaner replacement object");
-    }
-
-    protected final ReentrantLock getOrCreateMonitor(Object unreplacedObject, boolean createIfNotExisting) {
-        Object obj;
-        if (unreplacedObject == CLEANER_CLASS) {
+    protected static Object replaceObject(Object unreplacedObject) {
+        if (unreplacedObject instanceof DynamicHub) {
             /*
-             * Workaround for jdk.internal.ref.Cleaner when cleaners do not run in a separate
-             * thread. Cleaner uses static synchronized methods. Since classes (= DynamicHub) never
-             * have a monitor slot, static synchronized methods always use the additionalMonitors
-             * map. When a Cleaner then runs at a time where the application thread already holds
-             * the additionalMonitorsLock, i.e., when a GC runs while allocating a monitor in
-             * getOrCreateMonitorFromMap(), a disallowed recursive locking of additionalMonitorsLock
-             * would happen. Note that CLEANER_REPLACEMENT is an Object which always has a monitor
-             * slot. This workaround will be removed by GR-35898 when we have a better
-             * implementation of static synchronized methods.
+             * Classes (= DynamicHub) never have a monitor slot because they must be immutable.
+             * Since the companion object is never exposed to user code, we can use it as a
+             * replacement object that is mutable and is marked to always have a monitor slot.
              */
-            obj = CLEANER_REPLACEMENT;
-        } else {
-            obj = unreplacedObject;
+            return ((DynamicHub) unreplacedObject).getCompanion();
         }
+        return unreplacedObject;
+    }
 
+    protected final ReentrantLock getOrCreateMonitor(Object unreplacedObject, boolean createIfNotExisting) {
+        Object obj = replaceObject(unreplacedObject);
         assert obj != null;
         int monitorOffset = getMonitorOffset(obj);
         if (monitorOffset != 0) {