On Wed, 19 Jun 2024 15:06:25 GMT, Axel Boldt-Christmas <abold...@openjdk.org> 
wrote:

>> ClassLoaderDataGraph provides APIs for walking different metadata. All the 
>> iterators which are not designed to be used by the GC also keep the holder 
>> of the CLDs alive and by extensions keeps all metadata alive. This is 
>> problematic for concurrent GC as it keeps otherwise unreachable classes from 
>> being unloaded and the respective metadata freed. 
>> 
>> This patch changes the default iteration behaviour to not keep the holder 
>> alive, with the exception of `loaded_classes_do` (renamed 
>> `loaded_classes_do_keepalive`) and `modules_do` (renamed 
>> `modules_do_keepalive`) which is used by jvmti APIs that requires that the 
>> holder is kept alive.
>> 
>> All other uses consumes all the metadata it queries during its safepoint or 
>> before releasing the `ClassLoaderDataGraph_lock`. 
>> 
>> Before this change some jcmd, new jfr chunks and some jfr events, all of 
>> which consumed these APIs, could cause class unloading to not occur. 
>> 
>> Been running our internal stress test in an even more stressful mode which 
>> without this patch reproduces the metaspace OOME 
>> [JDK-8326005](https://bugs.openjdk.org/browse/JDK-8326005) consistently 
>> within a few hours. And after this patch it does not.
>> 
>> Currently running tier1-tier8 testing.
>
> Axel Boldt-Christmas has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Rename and comment SystemDictionary::methods_do

If the default is to not keep the CLD alive, I don't like that we need the 
details of the side effect in the name.  Just call it classes_do, etc.  I don't 
care about no-keepalive in all these callers, if that's the right answer for 
most of these callers.  Put the side effect in the name of the exceptional 
cases.

We had the iterator keep things alive for safety in the cases where we could 
have the CLD unload while we were looking at it.  In all these cases, this 
wasn't needed?  Still have to work through that.

-------------

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19769#pullrequestreview-2130887817

Reply via email to