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

[Florian David]

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