On Fri, 28 Apr 2023 12:48:44 GMT, Stefan Johansson <sjoha...@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
