From e31baf0a7f1bb7db6e7fb37434d19708a133b5b0 Mon Sep 17 00:00:00 2001 From: Christian Wimmer Date: Wed, 5 Jan 2022 13:35:26 -0800 Subject: [PATCH] Do not use secondary monitor map for locking on Class --- substratevm/CHANGELOG.md | 3 ++ .../com/oracle/svm/core/hub/DynamicHub.java | 42 ++++++++--------- .../svm/core/hub/DynamicHubCompanion.java | 35 +++++++++------ .../oracle/svm/core/jdk/RandomAccessors.java | 7 --- .../monitor/MultiThreadedMonitorSupport.java | 45 +++++++------------ 5 files changed, 61 insertions(+), 71 deletions(-) diff --git a/substratevm/CHANGELOG.md b/substratevm/CHANGELOG.md index 52a8bfd8bebc..cb4ae24f1abe 100644 --- a/substratevm/CHANGELOG.md +++ b/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. diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHub.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHub.java index bd9c4642392e..9fc0e2d3b424 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHub.java +++ b/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 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 diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHubCompanion.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHubCompanion.java index 8218432a3226..13fad05a6c4a 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHubCompanion.java +++ b/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,6 +37,8 @@ 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; @@ -46,20 +46,29 @@ 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 newDeclaredMethods = new ArrayList<>(Arrays.asList(hub.rd.declaredMethods)); List newPublicMethods = new ArrayList<>(Arrays.asList(hub.rd.publicMethods)); diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/RandomAccessors.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/RandomAccessors.java index 1c3d3fd0c49d..5ca522dff913 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/RandomAccessors.java +++ b/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) { diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java index 3842e458da32..605c55f2f6e6 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java +++ b/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) {