RFR(S): 8227597: [fastdbg build] Arena::inc_bytes_allocated should get inlined

[Doerr, Martin]

Hi Thomas,

thanks for looking into this issue.

Note that the counter is already non-atomic. We’re not using Atomic::add. It’s not supposed to be precise. We only use Atomic::load + store to avoid word-tearing on 32 bit.
For 64 bit platforms, my latest version only removes the “volatile” treatment on x86 and SPARC which is part of the Atomic::load + store.

But I’d also appreciate to remove the functionaliy.

Best regards,
Martin


From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Freitag, 12. Juli 2019 12:05
To: Doerr, Martin <martin.doerr at sap.com>
Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-dev at openjdk.java.net; Baesken, Matthias <matthias.baesken at sap.com>
Subject: Re: RFR(S): 8227597: [fastdbg build] Arena::inc_bytes_allocated should get inlined

Hi Martin,

I would think using it non-atomically makes the counter less useful, since you may loose concurrent updates, and since this a hot path (arena malloc) we may see a counter which is smaller than reality.

But I wonder if we even need it at all. This is the counter of bytes allocated from arena chunks, in total. We already have bytes-in-arena-chunks, via NMT. I guess that this counter will be trailing bytes-in-arena-chunks quite closely. For the purpose of "how much memory is allocated in arenas" the NMT counter works better.

I also assume that this counter was introduced when there was no NMT to track memory.

This counter is only used when printing allocation stats with PrintMallocStatistics. I would even think about removing that PrintMallocStatistics functionality altogether, since it is better covered with NMT.

For now I would just remove the counter, rather than making it non-atomic.

Cheers, Thomas

On Fri, Jul 12, 2019 at 11:48 AM Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi Götz and Matthias,

thanks for reviewing.

Right, using a simple addition instead of the atomics may allow more optimizations.
If nobody insists on treating the statistics values as volatile, I prefer this version:
http://cr.openjdk.java.net/~mdoerr/8227597_DBG_Inline_inc_bytes_allocated/webrev.01/

Best regards,
Martin


> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Freitag, 12. Juli 2019 10:26
> To: Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>; hotspot-runtime-
> dev at openjdk.java.net<mailto:dev at openjdk.java.net>
> Cc: Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>>; Claes Redestad
> <claes.redestad at oracle.com<mailto:claes.redestad at oracle.com>>
> Subject: RE: RFR(S): 8227597: [fastdbg build] Arena::inc_bytes_allocated
> should get inlined
>
> Hi Martin,
>
> thanks for looking at the timeouts we get with the jtreg tests
> on ppc.  Inlining inc_bytes_allocated looks like a step forward.
>
> But why do you remove the #if in inc_stat_counter()?
> It is not there because it's not implemented on other platforms,
> but because SPARC and X86 have (had) 32-bit variants.
> Actually, your change should slow down the code on
> PPC & others.
>
> I think the right #define here is #ifndef LP64.
> And you now need that in inc_bytes_allocated, too.
>
> Best,
>   Goetz.
>
>
> > -----Original Message-----
> > From: Doerr, Martin
> > Sent: Donnerstag, 11. Juli 2019 17:32
> > To: hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>
> > Cc: Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>>; Lindenmaier, Goetz
> > <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>; Claes Redestad
> <claes.redestad at oracle.com<mailto:claes.redestad at oracle.com>>
> > Subject: RFR(S): 8227597: [fastdbg build] Arena::inc_bytes_allocated should
> > get inlined
> >
> > Hi,
> >
> >
> >
> > the simple function Arena::inc_bytes_allocated can be found as CPU
> consuming
> > when profiling the fastdbg build. It is located in a cpp file.
> > It should better get inlined to improve the performance of the fastdbg VM
> > which is important for complex tests.
> > In addition, atomic 8-Byte load and store functions are available on all
> > platforms, so the "#if defined ..." can get removed.
> >
> >
> >
> > Here's my proposal:
> >
> >
> http://cr.openjdk.java.net/~mdoerr/8227597_DBG_Inline_inc_bytes_allocat
> ed
> > /webrev.00/
> >
> >
> >
> > Feedback is welcome.
> >
> >
> >
> > Best regards,
> >
> > Martin
> >
> >