review request (S) - 8057818: collect allocation context stats

[Mikael Gerdin]

Hi John,

On Tuesday 09 September 2014 10.08.51 John Coomes wrote:
> Mikael Gerdin (mikael.gerdin at oracle.com) wrote:
> > Hi John,
> > 
> > On Tuesday 09 September 2014 00.00.58 John Coomes wrote:
> > > Hi,
> > > 
> > > Please review an interface for collecting allocation context
> > > statistics at g1 gc pauses:
> > > 
> > > 8057818: collect allocation context statistics at gc pauses
> > > 
> > > http://cr.openjdk.java.net/~jcoomes/8u/8u40/8057818-stats-collect/
> > 
> > Why don't you inline the AllocationContextStats inside the class
> > declaration instead? It looks weird that the class just above uses that
> > pattern and this class doesn't. You don't really need to declare an
> > inline emtpy default constructor.
> 
> Hi Mikael,
> 
> Thanks for taking a look.  I tend to put most/all method bodies
> outside the class decl to keep the decl readable.  This is based at
> least in part on what I've seen in CollectedHeap and G1CollectedHeap,
> which are both huge.  But these methods are completely trivial, and
> fit well within the decl so I'll move them.
> 
> > We usually keep the "&" reference operator next to the type, just like we
> > do> 
> > with pointer types:
> >   inline AllocationContextStats& allocation_context_stats();
> > 
> > and
> > 
> >   inline AllocationContextStats&
> >   G1CollectedHeap::allocation_context_stats() {
> Sure, I'll fix that.
> 
> > there's not really any point to keeping the implementation of
> > allocation_context_stats() in the .inline.hpp file since G1CollectedHeap
> > needs the full class declaration of AllocationContextStats to determine
> > its size since it's embedded in the G1CollectedHeap object.
> 
> There is one - shrinking g1CollectedHeap.hpp and the G1CollectedHeap
> class decl :-).  I there any harm having it in the .inline.hpp?

Not really any harm, but not having a simple getter inlined in the hpp file 
makes me suspect that there's more to the method than just returning the 
field.
In general I agree with keeping delcaration-inline methods short, but I think 
that simple getters should stay in the declaration.
I'm not going to press on the issue though, if you really want to then keep it 
in the inline.hpp file.

/Mikael

> 
> -John