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

[Thomas Stüfe]

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> 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>; hotspot-runtime-
> > dev at openjdk.java.net
> > Cc: Baesken, Matthias <matthias.baesken at sap.com>; Claes Redestad
> > <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
> > > Cc: Baesken, Matthias <matthias.baesken at sap.com>; Lindenmaier, Goetz
> > > <goetz.lindenmaier at sap.com>; Claes Redestad
> > <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
> > >
> > >
>
>