Skip to content
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.

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

8078641: MethodHandle.asTypeCache can retain classes from unloading #5246

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
115 changes: 100 additions & 15 deletions src/java.base/share/classes/java/lang/invoke/MethodHandle.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@
package java.lang.invoke;


import jdk.internal.loader.ClassLoaders;
import jdk.internal.vm.annotation.IntrinsicCandidate;

import java.lang.constant.ClassDesc;
import java.lang.constant.Constable;
import java.lang.constant.DirectMethodHandleDesc;
import java.lang.constant.MethodHandleDesc;
import java.lang.constant.MethodTypeDesc;
import java.lang.ref.SoftReference;
import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;
Expand Down Expand Up @@ -449,12 +451,9 @@ public abstract class MethodHandle implements Constable {
@interface PolymorphicSignature { }

private final MethodType type;
/*private*/
final LambdaForm form;
// form is not private so that invokers can easily fetch it
/*private*/
MethodHandle asTypeCache;
// asTypeCache is not private so that invokers can easily fetch it
/*private*/ final LambdaForm form; // form is not private so that invokers can easily fetch it
private MethodHandle asTypeCache;
private SoftReference<MethodHandle> asTypeSoftCache;

private byte customizationCount;

Expand Down Expand Up @@ -855,34 +854,120 @@ public Object invokeWithArguments(java.util.List<?> arguments) throws Throwable
* @throws WrongMethodTypeException if the conversion cannot be made
* @see MethodHandles#explicitCastArguments
*/
public MethodHandle asType(MethodType newType) {
public final MethodHandle asType(MethodType newType) {
// Fast path alternative to a heavyweight {@code asType} call.
// Return 'this' if the conversion will be a no-op.
if (newType == type) {
return this;
}
// Return 'this.asTypeCache' if the conversion is already memoized.
MethodHandle atc = asTypeCached(newType);
if (atc != null) {
return atc;
MethodHandle at = asTypeCached(newType);
if (at != null) {
return at;
}
return asTypeUncached(newType);
return setAsTypeCache(asTypeUncached(newType));
}

private MethodHandle asTypeCached(MethodType newType) {
MethodHandle atc = asTypeCache;
if (atc != null && newType == atc.type) {
return atc;
return atc; // cache hit
}
SoftReference<MethodHandle> softCache = asTypeSoftCache;
if (softCache != null) {
atc = softCache.get();
if (atc != null && newType == atc.type) {
return atc; // soft cache hit
}
}
return null;
}

private MethodHandle setAsTypeCache(MethodHandle at) {
// Don't introduce a strong reference in the cache if newType depends on any class loader other than
// current method handle already does to avoid class loader leaks.
if (isSafeToCache(at.type)) {
asTypeCache = at;
} else {
asTypeSoftCache = new SoftReference<>(at);
}
return at;
}

/** Override this to change asType behavior. */
/*non-public*/
MethodHandle asTypeUncached(MethodType newType) {
if (!type.isConvertibleTo(newType))
throw new WrongMethodTypeException("cannot convert "+this+" to "+newType);
return asTypeCache = MethodHandleImpl.makePairwiseConvert(this, newType, true);
if (!type.isConvertibleTo(newType)) {
throw new WrongMethodTypeException("cannot convert " + this + " to " + newType);
}
return MethodHandleImpl.makePairwiseConvert(this, newType, true);
}

/**
* Returns true if {@code newType} does not depend on any class loader other than current method handle already does.
* May conservatively return false in order to be efficient.
*/
private boolean isSafeToCache(MethodType newType) {
ClassLoader loader = getApproximateCommonClassLoader(type);
return keepsAlive(newType, loader);
}

/**
* Tries to find the most specific {@code ClassLoader} which keeps all the classes mentioned in {@code mt} alive.
* In the worst case, returns a {@code ClassLoader} which relates to some of the classes mentioned in {@code mt}.
*/
private static ClassLoader getApproximateCommonClassLoader(MethodType mt) {
ClassLoader loader = mt.rtype().getClassLoader();
for (Class<?> ptype : mt.ptypes()) {
ClassLoader ploader = ptype.getClassLoader();
if (isAncestorLoaderOf(loader, ploader)) {
loader = ploader; // pick more specific loader
} else {
// Either loader is a descendant of ploader or loaders are unrelated. Ignore both cases.
// When loaders are not related, just pick one and proceed. It reduces the precision of keepsAlive, but
// doesn't compromise correctness.
}
}
return loader;
}

/* Returns true when {@code loader} keeps components of {@code mt} reachable either directly or indirectly through the loader delegation chain. */
private static boolean keepsAlive(MethodType mt, ClassLoader loader) {
for (Class<?> ptype : mt.ptypes()) {
if (!keepsAlive(ptype, loader)) {
return false;
}
}
return keepsAlive(mt.rtype(), loader);
}

/* Returns true when {@code loader} keeps {@code cls} either directly or indirectly through the loader delegation chain. */
private static boolean keepsAlive(Class<?> cls, ClassLoader loader) {
ClassLoader defLoader = cls.getClassLoader();
if (isBuiltinLoader(defLoader)) {
return true; // built-in loaders are always reachable
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for special case here. isAncestorLoaderOf(defLoader, loader) already handles this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though the check is redundant, I find the current version clearer.

return isAncestorLoaderOf(defLoader, loader);
}

private static boolean isAncestorLoaderOf(ClassLoader ancestor, ClassLoader descendant) {
// Assume built-in loaders are interchangeable and all custom loaders delegate to one of them.
if (isBuiltinLoader(ancestor)) {
return true;
}
// Climb up the descendant chain until a built-in loader is encountered.
for (ClassLoader loader = descendant; !isBuiltinLoader(loader); loader = loader.getParent()) {
if (loader == ancestor) {
return true;
}
}
return false; // no direct relation between loaders is found
}

private static boolean isBuiltinLoader(ClassLoader loader) {
return loader == null ||
loader == ClassLoaders.platformClassLoader() ||
loader == ClassLoaders.appClassLoader();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -493,12 +493,12 @@ public MethodHandle asTypeUncached(MethodType newType) {
if (newArity == collectArg+1 &&
type.parameterType(collectArg).isAssignableFrom(newType.parameterType(collectArg))) {
// if arity and trailing parameter are compatible, do normal thing
return asTypeCache = asFixedArity().asType(newType);
return asFixedArity().asType(newType);
}
// check cache
MethodHandle acc = asCollectorCache;
if (acc != null && acc.type().parameterCount() == newArity)
return asTypeCache = acc.asType(newType);
return acc.asType(newType);
// build and cache a collector
int arrayLength = newArity - collectArg;
MethodHandle collector;
Expand All @@ -509,7 +509,7 @@ public MethodHandle asTypeUncached(MethodType newType) {
throw new WrongMethodTypeException("cannot build collector", ex);
}
asCollectorCache = collector;
return asTypeCache = collector.asType(newType);
return collector.asType(newType);
}

@Override
Expand Down Expand Up @@ -737,7 +737,7 @@ public MethodHandle asTypeUncached(MethodType newType) {
} else {
wrapper = newTarget; // no need for a counting wrapper anymore
}
return (asTypeCache = wrapper);
return wrapper;
}

boolean countDown() {
Expand Down Expand Up @@ -1213,7 +1213,7 @@ protected MethodHandle getTarget() {
public MethodHandle asTypeUncached(MethodType newType) {
// This MH is an alias for target, except for the MemberName
// Drop the MemberName if there is any conversion.
return asTypeCache = target.asType(newType);
return target.asType(newType);
}
}

Expand Down Expand Up @@ -1276,7 +1276,7 @@ Object intrinsicData() {
public MethodHandle asTypeUncached(MethodType newType) {
// This MH is an alias for target, except for the intrinsic name
// Drop the name if there is any conversion.
return asTypeCache = target.asType(newType);
return target.asType(newType);
}

@Override
Expand Down