On Tue, 18 Jun 2024 12:25:36 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. Changes requested by eosterlund (Reviewer). 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. src/hotspot/share/prims/jvmtiEnvBase.cpp line 2342: > 2340: > 2341: // Iterate over all the modules loaded to the system. > 2342: ClassLoaderDataGraph::modules_do_keepalive(&do_module); Looks like this code exposes an OopHandle backed by the CLD handle area, which isn't a strong root that the GC will start tracing from. So it would seem that we need to keep these oops alive somehow. src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 108: > 106: // and collect them using the LoadedClassesClosure > 107: MutexLocker mcld(ClassLoaderDataGraph_lock); > 108: ClassLoaderDataGraph::loaded_classes_do_keepalive(&closure); This one looks like it might not be safe to exposes without keeping the classes alive. ------------- PR Review: https://git.openjdk.org/jdk/pull/19769#pullrequestreview-2125542310 PR Review Comment: https://git.openjdk.org/jdk/pull/19769#discussion_r1644492836 PR Review Comment: https://git.openjdk.org/jdk/pull/19769#discussion_r1644478958 PR Review Comment: https://git.openjdk.org/jdk/pull/19769#discussion_r1644479082
