review request (S) - 8057818: collect allocation context stats
Mikael Gerdin
mikael.gerdin at oracle.com
Tue Sep 9 18:19:12 UTC 2014
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
More information about the hotspot-gc-dev
mailing list