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

[Jon Masamitsu]

On 08/10/2015 06:01 AM, Thomas Schatzl wrote:
> 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.

Thanks.

>
>> 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.

Yes, I expected the function to get the string.  Thanks.

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

The eternal question for which there is no one good answer.  Your
call.

>
>> 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.

Thanks.
>
>>     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.

Thanks for the change.  I hesitated a bit about using "direct" because
CMS uses "direct" to mean objects allocated out of the CMS gen
as opposed to promoted to the CMS gen but  I think I like direct better
still.

Jon
>
>>>     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
>
>