On Tue, 18 Jun 2024 13:42:24 GMT, Erik Österlund <eosterl...@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. > > src/hotspot/share/classfile/classLoaderDataGraph.cpp line 245: > >> 243: // Iterating over the CLDG needs to be locked because >> 244: // unloading can remove entries concurrently soon. >> 245: class ClassLoaderDataGraph::ClassLoaderDataGraphIterator : public >> StackObj { > > If we now have only a no keepalive variation of this iterator implementation, > then perhaps it would be a good idea to have a comment here making it clear > that 1) this iterator does not keep the metadata alive and hence that 2) it > is up to the user to keep oops alive manually if they are to be exposed in > the object graph, or we will crash. Yes, let's add a comment and change the name to make this clear, even tough `ClassLoaderDataGraphIterator` is internal to `ClassLoaderDataGraph`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19769#discussion_r1644735596
