On Tue, 15 Sep 2026 17:08:29 GMT, Dan Heidinga <[email protected]> wrote:
> [Picking up and finishing off https://github.com/openjdk/jdk/pull/32519 for > Coleen] > > Remove the upcall to addClass during class loading. The comment says it's > only so GC can keep classes alive while the class loader is alive. We have > other ways to do that. There were some JVMTI tests in the past that failed > without this vector but today seems to be only one test. Maybe there's some > code that has a dependency on this in heap walking. > Tested tier1-6 > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Thanks for picking this one up Dan. A few minor comments/suggestions. src/hotspot/share/classfile/loaderConstraints.cpp line 445: > 443: } > 444: > 445: // Checks that a class wasn't added to the table then class loading > subsequently failed for this class, Suggestion: // Checks that a class that failed to load wasn't added to the table. src/hotspot/share/classfile/loaderConstraints.cpp line 448: > 446: // so we don't have a dangling pointer to InstanceKlass in the > LoaderConstraintTable. > 447: void LoaderConstraintTable::check_failed_loaded_klass(InstanceKlass* > klass, > 448: ClassLoaderData* > loader) { Not sure this really carries its weight any more as it doesn't actually need to do anything. Based on the comments once we have added to the constraint table it should not be possible for there to be any exceptions that would require it to be removed again. test/hotspot/jtreg/serviceability/HeapDump/ClassLoaderFieldsTest.java line 50: > 48: * @summary Verifies heap dump contains a reference to a java.lang.Class > instance from its ClassLoader > 49: * @library /test/lib > 50: * @run driver ClassLoaderFieldsTest I don't quite understand the name of the test - what "fields" are we referring to? test/hotspot/jtreg/serviceability/HeapDump/ClassLoaderFieldsTest.java line 60: > 58: cob.aload(0) > 59: .invokespecial(CD_Object, INIT_NAME, MTD_void) > 60: .return_()) Suggestion: clb.withFlags(ACC_PUBLIC) .withMethodBody(INIT_NAME, MTD_void, ACC_PUBLIC, cob -> cob.aload(0) .invokespecial(CD_Object, INIT_NAME, MTD_void) .return_()) test/hotspot/jtreg/serviceability/HeapDump/ClassLoaderFieldsTest.java line 82: > 80: LingeredApp.main(args); > 81: Reference.reachabilityFence(ldr); > 82: Reference.reachabilityFence(test); Surely only one is needed. If the class is reachable then so is its loader; and vice-versa. test/hotspot/jtreg/serviceability/jvmti/FollowReferences/ClassLoader/ClassLoaderTest.java line 50: > 48: cob.aload(0) > 49: .invokespecial(CD_Object, INIT_NAME, MTD_void) > 50: .return_()) Suggestion: clb.withFlags(ACC_PUBLIC) .withMethodBody(INIT_NAME, MTD_void, ACC_PUBLIC, cob -> cob.aload(0) .invokespecial(CD_Object, INIT_NAME, MTD_void) .return_()) test/hotspot/jtreg/serviceability/jvmti/FollowReferences/ClassLoader/ClassLoaderTest.java line 85: > 83: > 84: Reference.reachabilityFence(ldr); > 85: Reference.reachabilityFence(test); Again only one needed ------------- PR Review: https://git.openjdk.org/jdk/pull/32890#pullrequestreview-5217450425 PR Review Comment: https://git.openjdk.org/jdk/pull/32890#discussion_r4021609697 PR Review Comment: https://git.openjdk.org/jdk/pull/32890#discussion_r4021605087 PR Review Comment: https://git.openjdk.org/jdk/pull/32890#discussion_r4021641579 PR Review Comment: https://git.openjdk.org/jdk/pull/32890#discussion_r4021629155 PR Review Comment: https://git.openjdk.org/jdk/pull/32890#discussion_r4021633498 PR Review Comment: https://git.openjdk.org/jdk/pull/32890#discussion_r4021645164 PR Review Comment: https://git.openjdk.org/jdk/pull/32890#discussion_r4021648576
