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

John Coomes John.Coomes at oracle.com
Tue Sep 9 17:08:51 UTC 2014


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?

-John



More information about the hotspot-gc-dev mailing list