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

[Florian David]

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
>