11u testing (was: Re: [11u] RFR: 8258414: OldObjectSample events too expensive)

Thu Apr 22 05:25:06 UTC 2021

Should have had my first coffee before posting; Aleksey was not the creator
of the wiki page, so I add Christoph...

On Thu, Apr 22, 2021 at 7:19 AM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> Hi guys,
>
> Errors happen, that is normal. But I wonder if we need some clearer
> standards for testing downported patches? Currently the only requirement is
> to run tier1 tests, but nothing is mentioned about the configuration and
> the platforms to be tested. 11u is a stable maintenance release and in
> theory it should have a higher, not a lower bar for testing than the head
> release.
>
> Surely testing just release is not enough, the debug builds need to be
> tested too. Then, arguably one should test on more than one platform. It's
> fine for individual contributors which don't have the resources, but larger
> companies should be able to test 11u patches on multiple platforms.
> Otherwise the burden of running the full gamut of tests and analysing
> accumulated errors lies overly by the 11u maintainers.
>
> What do you think?
>
> (Ccing Aleksey as the maintainer of
> https://wiki.openjdk.java.net/display/JDKUpdates/How+to+contribute+a+fix).
>
> Cheers, Thomas
>
>
> On Wed, Apr 21, 2021 at 7:42 PM Hohensee, Paul <hohensee at amazon.com>
> wrote:
>
>> Hi, Jaroslav.
>>
>> This push (
>> https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/b103107d7ceb)
>> broke the debug builds, vis
>>
>> /home/hohensee/workspaces/jdk11u-dev/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp:298:14:
>> error: ‘JfrJavaSupport’ has not been declared
>>    DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(thread);)
>>
>> The fix is
>>
>> diff -r b103107d7ceb
>> src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp
>> ---
>> a/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp
>> Wed Apr 21 12:24:28 2021 +0200
>> +++
>> b/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp
>> Wed Apr 21 17:40:23 2021 +0000
>> @@ -24,6 +24,7 @@
>>
>>  #include "precompiled.hpp"
>>  #include "jfr/jfrEvents.hpp"
>> +#include "jfr/jni/jfrJavaSupport.hpp"
>>  #include "jfr/leakprofiler/chains/edgeStore.hpp"
>>  #include "jfr/leakprofiler/chains/objectSampleMarker.hpp"
>>  #include "jfr/leakprofiler/checkpoint/objectSampleCheckpoint.hpp"
>>
>> Thanks,
>> Paul
>>
>> -----Original Message-----
>> From: jdk-updates-dev <jdk-updates-dev-retn at openjdk.java.net> on behalf
>> of Jaroslav Bachorík <jaroslav.bachorik at datadoghq.com>
>> Date: Monday, April 12, 2021 at 1:40 AM
>> To: Florian David <florian.david 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
>>
>> 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
>>
>>