RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared

[Serguei Spitsyn]

On Fri, 28 Apr 2023 12:48:44 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

> Hi all, 
> 
> Please review this change to avoid CleanClassLoaderDataMetaspaces safepoint when there is nothing that can be cleaned up.
> 
> **Summary**
> When transforming/redefining classes a previous version list is linked together in the InstanceKlass. The original class is added to this list if it is still used or shared. The difference between shared and used is not currently noted. This leads to a problem when doing concurrent class unloading, because during that we postpone some potential work to a safepoint (since we are not in one). This is the CleanClassLoaderDataMetaspaces and it is triggered by the ServiceThread if there is work to be done, for example if InstanceKlass::_has_previous_versions is true.
> 
> Since we currently does not differentiate between shared and "in use" we always set _has_previous_versions if anything is on this list. This together with the fact that shared previous versions should never be cleaned out leads to this safepoint being triggered after every concurrent class unloading even though there is nothing that can be cleaned out.
> 
> This can be avoided by making sure the _previous_versions list is only cleaned when there are non-shared classes on it. This change renames `_has_previous_versions` to `_clean_previous_versions` and only updates it if we have non-shared classes on the list.
> 
> **Testing**
> * A lot of manual testing verifying that we do get the safepoint when we should. 
> * Added new test to verify expected behavior by parsing the logs. The test uses JFR to trigger redefinition of some shared classes (when -Xshare:on).
> * Mach5 run of new test and tier 1-3

Thank you for taking care about it.
I've posted a couple of comments but it it looks good anyway.
Thanks,
Serguei

test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineSharedClassJFR.java line 94:

> 92:                     .shouldNotContain("scratch class added; one of its methods is on_stack.")
> 93:                     .shouldHaveExitValue(0);
> 94:                 return;

The fragments 61-74 and 79-93 have a big common part which can be a good base for a refactoring.
But it can be not worth it. So, I leave it up to you.

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

PR Review: https://git.openjdk.org/jdk/pull/13716#pullrequestreview-1408498631
PR Review Comment: https://git.openjdk.org/jdk/pull/13716#discussion_r1182156381