[11u] RFR: 8258414: OldObjectSample events too expensive
Markus Gronlund
markus.gronlund at oracle.com
Thu May 27 11:54:19 UTC 2021
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