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

Florian David florian.david at datadoghq.com
Fri May 28 08:24:24 UTC 2021 

Thanks Markus for the review.

Here is an updated webrev addressing previous comments. I rebuilt the
webrev with new changes in fastdebug just in case.

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

I'm updating the Fix request in the bug report accordingly.

Thanks,
Florian


On Thu, May 27, 2021 at 1:54 PM Markus Gronlund <markus.gronlund at oracle.com>
wrote:

> Hi Florian and Jaroslav,
>
> objectSampleCheckpoint.cpp:
> ...
> 26 #include "jfr/jni/jfrJavaSupport.hpp"
> and
> 297   JavaThread* const thread = JavaThread::current();
> 298   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(thread);)
>
> All this can be removed, because its only used to verify the single state
> there is in 11, i.e. "_thread_in_vm." Only in later versions we explicitly
> transition between "_thread_in_native" and "_thread_in_vm".
>
> 287     ResourceMark rm;
> and
> 293 // caller needs ResourceMark
>
> With the ResourceMark introduced in 287, the comment at 293 can be removed.
>
> jfrStackTraceRepository.hpp:
> ...
> 71   const JfrStackTrace* lookup(unsigned int hash, traceid id) const;
>
> Can be removed.
>
> Otherwise looks good, don't need an updated webrev.
>
> Thank you
> Markus
>
> -----Original Message-----
> From: jdk-updates-dev <jdk-updates-dev-retn at openjdk.java.net> On Behalf
> Of Florian David
> Sent: den 19 maj 2021 16:26
> To: Jaroslav Bachorík <jaroslav.bachorik at datadoghq.com>
> Cc: jdk-updates-dev <jdk-updates-dev at openjdk.java.net>; Marcus Hirt <
> marcus.hirt at datadoghq.com>
> Subject: Re: [11u] RFR: 8258414: OldObjectSample events too expensive
>
> Hi, I'm re-sending this email since Martin Doerr didn't receive it and
> found it with the mailing list archives, so maybe others miss it. Martin
> reported that he doesn't see test errors anymore with this webrev.
>
> 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 Fri, May 7, 2021 at 6:34 PM Florian David <florian.david at datadoghq.com>
> wrote:
>
> > Hi all,
> >
> > Please find an update to the backport. Comments related to
> > https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2021-April/005
> > 880.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
> >>
> >
>

More information about the jdk-updates-dev mailing list