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

Wed May 19 14:25:51 UTC 2021

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/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
>>
>