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

[Jaroslav Bachorík]

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