Skip to content

[GR-35898] Do not use secondary monitor map for locking on Class. #4179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 8, 2022
Merged

[GR-35898] Do not use secondary monitor map for locking on Class. #4179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions substratevm/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
42 changes: 19 additions & 23 deletions substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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()) |
Expand All @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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, ")"));
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -1132,7 +1128,7 @@ private Field[] privateGetPublicFields() {

@Substitute
Method[] privateGetPublicMethods() {
return companion.get().getCompleteReflectionData().publicMethods;
return companion.getCompleteReflectionData(this).publicMethods;
}

@KeepOriginal
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -1330,7 +1326,7 @@ public Object[] getSigners() {

@Substitute
public ProtectionDomain getProtectionDomain() {
return companion.get().getProtectionDomain();
return companion.getProtectionDomain();
}

@TargetElement(onlyWith = JDK17OrLater.class)
Expand All @@ -1340,7 +1336,7 @@ private ProtectionDomain protectionDomain() {
}

void setProtectionDomainAtRuntime(ProtectionDomain protectionDomain) {
companion.get().setProtectionDomain(protectionDomain);
companion.setProtectionDomain(protectionDomain);
}

@Substitute
Expand Down
35 changes: 22 additions & 13 deletions substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHubCompanion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
}
Expand All @@ -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));
Expand Down
7 changes: 0 additions & 7 deletions substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/RandomAccessors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
45 changes: 17 additions & 28 deletions .../src/com.oracle.svm.core/src/com/oracle/svm/core/monitor/MultiThreadedMonitorSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down