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

Fri Apr 23 14:32:37 UTC 2021

Hi,

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.

I totally agree. This was my fault as I was lulled to a false sense of
safety by the GH actions which I completely forgot were not happening
for 11u.

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

IMO, a requirement to have a full spectrum of OS-HW combinations
covered before pushing a backport will basically mean that the
contributors without access to such resources will not be able/allowed
to push backports at all.
I am not saying it is wrong - just flagging the possible result.

Cheers,

-JB-

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