RFR: 8297427: Avoid keeping class loaders alive when executing ClassLoaderStatsVMOperation [v2]
David Holmes
dholmes at openjdk.org
Thu Dec 1 00:29:33 UTC 2022
On Wed, 30 Nov 2022 09:08:52 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:
>> Please review this change to avoid keeping classes alive only due to the `ClassLoaderStatsVMOperation`.
>>
>> **Summary**
>> The `ClassLoaderStatsVMOperation` is gathering statistics about the active class loaders in a safepoint. The way the `ClassLoaderDataGraph` is iterated will keep the class loaders live. This is not really needed since everything is done in a safepoint and nothing needs to be explicitly kept alive. This has not been a problem prior to concurrent class unloading in ZGC. With fully concurrent class unloading a `ClassLoaderStatsVMOperation` can occur during a collection and more classes than needed might be kept alive. This could in turn lead to premature Metaspace OOM.
>>
>> The solution is to not keep the class loaders alive due to the iteration in `ClassLoaderStatsVMOperation`.
>>
>> **Testing**
>> * Added a new test that covers the two different ways a class could previously be kept alive by the VM operation. The test passes after the fix but failed before.
>> * Mach5 tier 1-3
>
> Stefan Johansson has updated the pull request incrementally with three additional commits since the last revision:
>
> - Print object to ensure it is kept alive
> - Revert "Axel comments to use templates"
>
> This reverts commit 8800ef089b62cf173147b68adb2ee993b7e72980.
> - Revert "Missing include for minimal"
>
> This reverts commit 424e0f9d831279ba2d1986ebacb499f8d4a6c078.
A few drive-by comments on the test.
test/hotspot/jtreg/runtime/ClassUnload/UnloadTestDuringClassLoaderStatsVMOperation.java line 26:
> 24: /*
> 25: * @test Class unloading test while triggering execution of ClassLoaderStats VM operations
> 26: * @summary Make sure class unloading occur even if ClassLoaderStats VM operations are executed
Having a comment on `@test` and providing `@summary` seems redundant. I would just leave the summary.
You could/should(?) add a `@bug` line too.
test/hotspot/jtreg/runtime/ClassUnload/UnloadTestDuringClassLoaderStatsVMOperation.java line 30:
> 28: * @modules java.base/jdk.internal.misc
> 29: * @library /test/lib
> 30: * @library classes
You can put multiple libraries on one `@library` - could save some jtreg parsing time.
test/hotspot/jtreg/runtime/ClassUnload/UnloadTestDuringClassLoaderStatsVMOperation.java line 39:
> 37: import java.net.URLClassLoader;
> 38:
> 39: import jdk.test.lib.classloader.ClassUnloadCommon;
Should imports be sorted and grouped ie.
import java.net.URLClassLoader;
import jdk.test.lib.classloader.ClassUnloadCommon;
import jdk.test.whitebox.WhiteBox;
? Not sure if we have a style-guide for this.
test/hotspot/jtreg/runtime/ClassUnload/UnloadTestDuringClassLoaderStatsVMOperation.java line 55:
> 53: }
> 54: };
> 55: var clsThread = new Thread(task);
You don't need `task` here as you can just write the above as:
var clsThread = new Thread(() -> {
while (true) {
wb.forceClassLoaderStatsSafepoint();
}
});
test/hotspot/jtreg/runtime/ClassUnload/UnloadTestDuringClassLoaderStatsVMOperation.java line 75:
> 73: ClassUnloadCommon.failIf(!wb.isClassAlive(className), className + " should be loaded and live");
> 74:
> 75: // Printing the object to ensure the class is kept alive. If the test
We should be using `Reference.reachabilityFence` to keep objects alive.
-------------
PR: https://git.openjdk.org/jdk/pull/11300
More information about the hotspot-dev
mailing list