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
Conversation
|
/contributor add plevart |
@iwanowww |
@iwanowww |
Webrevs
|
Looks good.
I guess it is not that common for the soft ref to get instantiated i.e. for the case of the ~common class loader of type
, MTC say, and the ~common classoader of newType
, NMTC say, then NMTC is not an ancestor of MTC.
It's possible that asTypeCache
and asTypeSoftCache
could both be non-null i.e. we don't null out one of them, buy does not seem a problem.
@iwanowww This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 112 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
Thanks for the reviews, Paul and Mandy. What do you think about the latest version? |
|
||
/* 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) { | ||
return keepsAlive(cls.getClassLoader(), loader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return keepsAlive(cls.getClassLoader(), loader); | |
ClassLoader defLoader = cls.getClassLoader(); | |
if (isBuiltinLoader(defLoader)) { | |
return true; // built-in loaders are always reachable | |
} | |
return keepsAlive(defLoader, loader); |
I think it's clearer to check if cls
is not defined by any builtin loader here and then check if loader
keeps cls
alive.
So keepsAlive(ClassLoader loader1, ClassLoader loader2)
is not needed and replace line 935 and 940 to keepsAlive(Class, ClassLoader)
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
Incorporated your suggestions along with some minor refactorings.
This looks good to me. For the change of |
It is not allowed to extend/subclass
|
} | ||
if (asTypeSoftCache != null) { | ||
atc = asTypeSoftCache.get(); | ||
if (newType == atc.type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NPE is possible here! act can be null as it is a result of SoftReference::get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed.
return atc; // cache hit | ||
} | ||
if (asTypeSoftCache != null) { | ||
atc = asTypeSoftCache.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NPE is possible here too! asTypeSoftCache is a non-volatile field which is read twice. First time in the if (...) condition, 2nd time in the line that de-references it to call .get(). This is a data-race since concurrent thread may be setting this field from null to non-null. Those two reads may get reordered. 1st read may return non-null while 2nd may return null. This can be avoided if the field is read just once by introducing a local variable to store its value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
return loader; | ||
} | ||
|
||
/* Returns true when {@code loader} keeps {@code mt} either directly or indirectly through the loader delegation chain. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, to be precise, loader can't keep mt alive. It would be better to say "keeps mt components alive" ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
ClassLoader defLoader = cls.getClassLoader(); | ||
if (isBuiltinLoader(defLoader)) { | ||
return true; // built-in loaders are always reachable | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This looks good to me now. |
Did we want this to use a soft reference or a weak reference? The original bug report mentions weak references, and the typical approach for preventing caches from holding onto things unnecessarily is to use weak references. I haven't thought through the implications of using soft instead of weak references. (Sorry if I missed any discussion on this issue.) |
Other MethodHandle/LambdaForm caches in On the other hand, Personally, I'm in favor |
I don't have a strong opinion on whether this should use a SoftReference or a WeakReference. (In fact, one might say that I have a phantom opinion.) The main thing was that the bug report mentioned WeakReference but the commit here uses SoftReferences, and I didn't see any discussion about this change. You gave some reasons above for why SoftReference is preferable, and that's ok with me. |
Thanks for the reviews, Mandy, Paul, Peter, and Stuart. /integrate |
Going to push as commit 21012f2.
Your commit was automatically rebased without conflicts. |
Sorry for the late reply as I was on vacation and just return today. Thanks for the clarification. I missed the fact that |
MethodHandle.asTypeCache
keeps a strong reference to adaptedMethodHandle
and it can introduce a class loader leak through itsMethodType
.Proposed fix introduces a 2-level cache (1 element each) where 1st level can only contain
MethodHandle
s which are guaranteed to not introduce any dependencies on new class loaders compared to the originalMethodHandle
. 2nd level is backed by aSoftReference
and is used as a backup when the result ofMethodHandle.asType()
conversion can't populate the higher level cache.The fix is based on the work made by Peter Levart @plevart back in 2015.
Testing: tier1 - tier6
Progress
Issue
Reviewers
Contributors
<plevart@openjdk.org>
<vlivanov@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5246/head:pull/5246
$ git checkout pull/5246
Update a local copy of the PR:
$ git checkout pull/5246
$ git pull https://git.openjdk.java.net/jdk pull/5246/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5246
View PR using the GUI difftool:
$ git pr show -t 5246
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5246.diff