RFR (L): 8073013: Add detailed information about PLAB memory usage

[Thomas Schatzl]

Hi Jon,

  thanks for the review. Comments inline.

On Fri, 2015-08-07 at 11:29 -0700, Jon Masamitsu wrote:
> Thomas,
> 
> Basically looks good.  Some questions around the edges.
> 
> http://cr.openjdk.java.net/~tschatzl/8073013/webrev/src/share/vm/gc/g1/g1CollectedHeap.hpp.frames.html
> 
> >   248   // Manages all kinds of allocations within regions. This excludes only
> >   249   // humongous object allocations.
> >   250   G1Allocator* _allocator;
> 
> Would this comment be more exact?
> 
> // Manages all allocations with regions except humongous object allocations.
> 

Done.

> http://cr.openjdk.java.net/~tschatzl/8073013/webrev/src/share/vm/gc/g1/g1EvacStats.cpp.html
> 
>    85 void G1EvacStats::send_obj_copy_mem_event(int for_gen) {
>    86   EventGCG1EvacuationMemoryStatistics e;
>    87   if (e.should_commit()) {
>    88     e.set_gcId(GCId::peek().id());
>    89     e.set_gen(for_gen);
> 
> Can the conversion of  InCSetState values to generation numbers be put into
> the send_obj_copy_mem_event()?
> 
> http://cr.openjdk.java.net/~tschatzl/8073013/webrev/src/share/vm/trace/trace.xml.frames.html
> 
>   318       <value type="UINT" field="gen" label="Generation" description="Generation these PLAB statistics are for"/>
> 
> 
> Can the "gen" field be a character string with "young" or "old" instead 
> of a number?

Did so in the new change. This obviates the need for a dedicated
conversion function, but adds a function to get the string.

Another option would be to have two separate events, not sure if this is
better.

> 
> http://cr.openjdk.java.net/~tschatzl/8073013/webrev/src/share/vm/gc/g1/g1EvacStats.hpp.html
> 
> Are these alternative comments correct?   Perhaps better for the less G1 
> knowledgeable?
> 
>    38   // Number of words allocated during evacuation failure in the regions that failed evacuation.
> 
> // Number of words in live objects remaining in regions that ultimately suffered an
> // evacuation failure.  This is used space in the regions when the regions are made old regions.
> 
> 
>    39   size_t _failure_used;
> 
>    40   // Number of words wasted during evacuation failure in the regions that failed evacuation.
> 
> // Number of words wasted in regions which failed evacuation.  This is the sum of space for
> // objects successfully copied out of the regions (now dead space) plus waste at the end of
> // regions.

Done.

> 
>    41   size_t _failure_waste;
> 
> 
> Not sure how ingrained the term "inline" allocation, but why "inline"?  It means that
> the allocation was directly into the region and not in a LAB in the region, right?
> Would _directly_allocated or _direct_allocations work for you?

Would be fine with me. In the most recent change for 8003237 I also
undid the renaming of that method from "inline" to "direct" so that
everything fits again.

I hope this makes the statistic more self-explaining, or at least less
confusing with other uses of "inline allocation".

I.e. afaik the term is used for allocation outside of PLAB using
additional inlined code.

Interestingly existing PLAB jfr events do not use the term inline
allocation either, the events have the suffix "OutsidePLAB" for object
promotion statistics.

> >    36   size_t _inline_allocated; // Number of words allocated directly into the regions.
> 
> That's all.

Thanks, your reviews help me a lot.

> 
> 
> On 8/6/2015 8:49 AM, Thomas Schatzl wrote:
> > Hi all,
> >
[...]
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8073013
> >
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8073013/webrev
> >
> > Testing:
> > jprt, a few aurora runs, local testing, lots of benchmarks mainly in
> > conjunction with the following changes.
> >

New webrev at:
http://cr.openjdk.java.net/~tschatzl/8073013/webrev.1 (full)
http://cr.openjdk.java.net/~tschatzl/8073013/webrev.0_to_1 (diff)

Thanks,
  Thomas