Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared [v2]
On Fri, 28 Apr 2023 15:57:49 GMT, Coleen Phillimore wrote: >> Stefan Johansson has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Test refactor >> - Serguei review > > This looks good. Thanks for all the testing and adding the new test. Thanks again @coleenp and @sspitsyn for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/13716#issuecomment-1534557035
Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared [v2]
On Wed, 3 May 2023 08:36:22 GMT, Stefan Johansson 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 > > Stefan Johansson has updated the pull request incrementally with two > additional commits since the last revision: > > - Test refactor > - Serguei review Thank you for the update. Looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13716#pullrequestreview-1411506472
Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared [v2]
> 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 Stefan Johansson has updated the pull request incrementally with two additional commits since the last revision: - Test refactor - Serguei review - Changes: - all: https://git.openjdk.org/jdk/pull/13716/files - new: https://git.openjdk.org/jdk/pull/13716/files/39c3a1c1..834174f9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13716&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13716&range=00-01 Stats: 47 lines in 5 files changed: 13 ins; 2 del; 32 mod Patch: https://git.openjdk.org/jdk/pull/13716.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13716/head:pull/13716 PR: https://git.openjdk.org/jdk/pull/13716
Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared [v2]
On Fri, 28 Apr 2023 15:57:49 GMT, Coleen Phillimore wrote: >> Stefan Johansson has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Test refactor >> - Serguei review > > This looks good. Thanks for all the testing and adding the new test. Thanks for the reviews @coleenp and @sspitsyn. Pushed two changes according to Sergueis suggestions. - PR Comment: https://git.openjdk.org/jdk/pull/13716#issuecomment-1532646363
Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared
On Fri, 28 Apr 2023 12:48:44 GMT, Stefan Johansson 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
Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared
On Fri, 28 Apr 2023 12:48:44 GMT, Stefan Johansson 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 src/hotspot/share/oops/instanceKlass.hpp line 718: > 716: > 717: private: > 718: static bool _clean_previous_versions; Nit: I'd suggest to name it as `_should_clean_previous_versions`. Then the corresponding function needs to be named as `should_clean_previous_versions()`. - PR Review Comment: https://git.openjdk.org/jdk/pull/13716#discussion_r1182136483
Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared
On Fri, 28 Apr 2023 12:48:44 GMT, Stefan Johansson 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 This looks good. Thanks for all the testing and adding the new test. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13716#pullrequestreview-1406222927
RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared
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 - Commit messages: - Add test wih JFR - 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared Changes: https://git.openjdk.org/jdk/pull/13716/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13716&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8306929 Stats: 139 lines in 6 files changed: 116 ins; 3 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/13716.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13716/head:pull/13716 PR: https://git.openjdk.org/jdk/pull/13716
