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

Mon May 31 11:59:47 UTC 2021

Hi,

Looks good,

I have only smoke-tested this on some available x64 platforms (Linux, Windows, Mac) – I have no status information about other platforms / targets.

Thanks
Markus

From: Florian David <florian.david at datadoghq.com>
Sent: den 28 maj 2021 10:24
To: Markus Gronlund <markus.gronlund at oracle.com>
Cc: Jaroslav Bachorík <jaroslav.bachorik at datadoghq.com>; 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

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<https://urldefense.com/v3/__https:/github.com/openjdk/jdk/pull/2780__;!!GqivPVa7Brio!O9kV0FCBlvBfCVtzqtS3fmEH-Jf8cHY1xqKhFa-P5HPKiYTkAIPbHw6zPRKGNKT8R-94$>

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<mailto: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<mailto: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<mailto:jaroslav.bachorik at datadoghq.com>>
Cc: jdk-updates-dev <jdk-updates-dev at openjdk.java.net<mailto:jdk-updates-dev at openjdk.java.net>>; Marcus Hirt <marcus.hirt at datadoghq.com<mailto: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<https://urldefense.com/v3/__https:/github.com/openjdk/jdk/pull/2780__;!!GqivPVa7Brio!O9kV0FCBlvBfCVtzqtS3fmEH-Jf8cHY1xqKhFa-P5HPKiYTkAIPbHw6zPRKGNKT8R-94$>

Thanks,
Florian DAVID

On Fri, May 7, 2021 at 6:34 PM Florian David <florian.david at datadoghq.com<mailto: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<https://urldefense.com/v3/__https:/github.com/openjdk/jdk/pull/2780__;!!GqivPVa7Brio!O9kV0FCBlvBfCVtzqtS3fmEH-Jf8cHY1xqKhFa-P5HPKiYTkAIPbHw6zPRKGNKT8R-94$>
>
> Thanks,
> Florian DAVID
>
> On Mon, Apr 12, 2021 at 10:38 AM Jaroslav Bachorík <
> jaroslav.bachorik at datadoghq.com<mailto: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<mailto: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<https://urldefense.com/v3/__https:/github.com/openjdk/jdk/pull/2780__;!!GqivPVa7Brio!O9kV0FCBlvBfCVtzqtS3fmEH-Jf8cHY1xqKhFa-P5HPKiYTkAIPbHw6zPRKGNKT8R-94$>
>> >
>> > Florian
>> >
>> > On Sun, Apr 4, 2021 at 8:33 PM Florian David <
>> florian.david at datadoghq.com<mailto: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<https://urldefense.com/v3/__https:/github.com/openjdk/jdk/pull/2780__;!!GqivPVa7Brio!O9kV0FCBlvBfCVtzqtS3fmEH-Jf8cHY1xqKhFa-P5HPKiYTkAIPbHw6zPRKGNKT8R-94$>
>> >>
>> >> Florian

<snip>