[11u] RFR: 8258414: OldObjectSample events too expensive

Fri May 7 16:34:00 UTC 2021

Hi all,

Please find an update to the backport. Comments related to
https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2021-April/005880.html

where the previous patch has broken the 11u build have been taken into
account:

- check_java_thread_in_native is replaced by check_java_thread_in_vm
in ObjectSampleCheckpoint::on_rotation
- ThreadInVMfromNative transition(thread) is removed. That check is part of
the original PR for JDK17 but since
some code in JFR was moved from JNI (in 11) to native in 16, this check is
not valid here.

Build and Tests:
- Linux release and fastdebug builds. Tested with make run-test-tier1 &&
make run-test TEST="jtreg:jdk/jfr/".
- Windows release and fastdebug builds. Tested with make run-test-tier1 &&
make run-test TEST="jtreg:jdk/jfr/". The
run-test-tier1 test suite did not complete entirely because of some issues
with paths being too long and
"vswhere.exe" not able to be run by the test on my machine. I would feel
more confident (especially Windows was broken
by the previous backport) if a maintainer could run the test suite on
Windows. The "jtreg:jdk/jfr/" test suite ran successfully.

Bug: https://bugs.openjdk.java.net/browse/JDK-8258414
webrev: http://cr.openjdk.java.net/~fdavid/8258414/webrev.03/
Original PR: https://github.com/openjdk/jdk/pull/2780

Thanks,
Florian DAVID

On Mon, Apr 12, 2021 at 10:38 AM Jaroslav Bachorík <
jaroslav.bachorik at datadoghq.com> wrote:

> Great, thanks!
>
> Reviewed!
>
> -JB-
>
> On Fri, Apr 9, 2021 at 11:49 PM Florian David
> <florian.david at datadoghq.com> wrote:
> >
> > After receiving feedback from Markus stating that:
> > > There is no need to attempt a lock replacement, because in 11, there
> is no concurrent class unloading. There, unloading only happens at a
> safepoint.
> > > With concurrent class unloading, there is a need to protect this list,
> but in 11 it is mutually exclusive and cannot be modified concurrently with
> the JFR Recorder thread calling "install_stack_traces(sampler").
> >
> > I removed the module lock and added an is_at_safepoint() assert.
> >
> > Update webrev link:
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8258414
> > webrev: http://cr.openjdk.java.net/~fdavid/8258414/webrev.02/
> > Original PR: https://github.com/openjdk/jdk/pull/2780
> >
> > Florian
> >
> > On Sun, Apr 4, 2021 at 8:33 PM Florian David <
> florian.david at datadoghq.com> wrote:
> >>
> >> Hi Jaroslav,
> >>
> >> Thanks for the review.
> >>
> >>>
> >>> -
> src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp
> >>>    L287 - IMO, the TODO is not necessary
> >>
> >> It's removed.
> >>
> >>>
> >>>    L293 - I think the comment `// caller needs ResourceMark` should
> >>> not be removed
> >>
> >> I added it back.
> >>
> >>>    L303 - Not sure if using Module_lock instead of
> >>> ClassLoaderDataGraph_lock is correct. Also, this change seems to be
> >>> bringing in changes unrelated to the patch.
> >>>               From what is happening in other places it would seem
> >>> that a safepoint should be asserted instead (or nothing should be
> >>> done).
> >>>               I will let Markus weigh in on this.
> >>
> >> No change for the moment.
> >>
> >>>
> >>> - src/hotspot/share/jfr/support/jfrAllocationTracer.cpp
> >>>    L38 - can this be completely removed?
> >>
> >> It's removed.
> >>
> >>>
> >>> - src/hotspot/share/jfr/support/jfrAllocationTracer.hpp
> >>>   L30 - I think `class JfrThreadLocal;` can also be removed
> >>>
> >> It's removed.
> >>
> >> Update webrev link:
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8258414
> >> webrev: http://cr.openjdk.java.net/~fdavid/8258414/webrev.01/
> >> Original PR: https://github.com/openjdk/jdk/pull/2780
> >>
> >> Florian
> >>
> >> On Mon, Mar 29, 2021 at 12:22 PM Jaroslav Bachorík <
> jaroslav.bachorik at datadoghq.com> wrote:
> >>>
> >>> Hi Florian,
> >>>
> >>> a few nits:
> >>> -
> src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp
> >>>    L287 - IMO, the TODO is not necessary
> >>>    L293 - I think the comment `// caller needs ResourceMark` should
> >>> not be removed
> >>>    L303 - Not sure if using Module_lock instead of
> >>> ClassLoaderDataGraph_lock is correct. Also, this change seems to be
> >>> bringing in changes unrelated to the patch.
> >>>               From what is happening in other places it would seem
> >>> that a safepoint should be asserted instead (or nothing should be
> >>> done).
> >>>               I will let Markus weigh in on this.
> >>> - src/hotspot/share/jfr/support/jfrAllocationTracer.cpp
> >>>    L38 - can this be completely removed?
> >>> - src/hotspot/share/jfr/support/jfrAllocationTracer.hpp
> >>>   L30 - I think `class JfrThreadLocal;` can also be removed
> >>>
> >>> Cheers,
> >>>
> >>> -JB-
> >>>
> >>>
> >>> On Wed, Mar 24, 2021 at 8:42 PM Florian David
> >>> <florian.david at datadoghq.com> wrote:
> >>> >
> >>> > Hi,
> >>> >
> >>> > Please review this 11u backport. It's very similar to the original
> patch,
> >>> > except for the class loader data graph lock and
> JfrKlassUnloading::sort()
> >>> > method which are not available in 11u.
> >>> >
> >>> > The missing lock has been replaced by locking the module_lock
> instead,
> >>> > as it is done in jfrcheckpointManager.cpp:363.
> >>> > JfrKlassUnloading class does not exist and thus sort() method is not
> replaced,
> >>> > I added a TODO instead.
> >>> >
> >>> > Bug: https://bugs.openjdk.java.net/browse/JDK-8258414
> >>> > webrev: http://cr.openjdk.java.net/~fdavid/8258414/webrev.00/
> >>> > Original PR: https://github.com/openjdk/jdk/pull/2780
> >>> >
> >>> > Testing:
> >>> > - Linux x86_64 tier1 tests
> >>> > - SPECvm 2008 on a 96 cores/192 Gib server. JFR recording size is
> 75.12 MB before the patch and 3.08 MB after the patch.
> >>> >
> >>> > Let me know what you think.
> >>> >
> >>> > Thanks,
> >>> > Florian
>