RFR: 8009032 - Implement evacuation info event

[Bengt Rutisson]

Hi Jesper,

Thanks for cleaning the naming up.

This looks good except that in gcTraceSend.cpp you need to move #include 
"gc_implementation/g1/evacuationInfo.hpp" into the #ifndef SERIALGC 
section. I don't think it will build for embedded in hsx24 otherwise and 
in hsx25 it may break if a dependency to any excluded file is ever added 
to evacuationInfo.hpp. In gcTrace.cpp you already have the include 
guarded by the #ifndef.

A couple of minor things in trace.xml:

I think the field name collectionSetRegions could be cSetRegions to be 
consistent with cSetUsedBefore and cSetUsedAfter.

As we discussed yesterday I am not convinced that the information in 
allocRegionsUsedAfter is worth having since it is just derived from 
allocRegionsUsedBefore and bytesCopied. But I'm ok with having it.

bytesCopied and regionsFreed are missing descriptions. Is this intended 
or do you want to add some explanation for these values as well.

Thanks,
Bengt

On 4/10/13 10:15 PM, Jesper Wilhelmsson wrote:
> Hi,
>
> New webrev available here:
> http://cr.openjdk.java.net/~jwilhelm/8009032/hotspot-webrev/
>
> All names are changed to reduce confusion.
> /Jesper
>
>
> Bengt Rutisson skrev 10/4/13 10:16 AM:
>>
>> Hi Jesper,
>>
>> This is better but I'm a bit confused about the data that we report.
>>
>> usedAllocRegionsBefore/used_alloc_regions_before
>> Does not seem to be the correct name. What you report here is the 
>> used bytes for
>> the one alloc region that we retained from the last GC.
>>
>> usedCSetAfter/used_collectionset_after
>> Does not seem to be the correct name. What you report is the used 
>> bytes for the
>> survivor regions. The old alloc regions are not included.
>>
>> usedAllocRegionsAfter/used_alloc_regions_after
>> Does not seem to be the correct name. What you report is the amount 
>> of bytes
>> that survived the collection. Either because it was copied (to alloc 
>> regions) or
>> because we got evacuation failure and it was left in place. This 
>> value also does
>> not include the value that you reported in 
>> "used_alloc_regions_before". I don't
>> think it should, but the current name kind of indicates that.
>>
>> regionsFreed/regions_freed
>> Does not seem to be the correct name. What you report is the number 
>> of regions
>> that had been freed just before the current region. I think the name 
>> may be
>> correct if you move the call to 
>> _evacuation_info.register_regions_freed() to
>> after the loop.
>>
>>
>> Code comments:
>>
>> g1CollectedHeap.cpp/hpp
>>
>> I would suggest to not have _evacuation_info as an instance variable in
>> G1CollectedHeap and instead have it as a local variable in
>> G1CollectedHeap::do_collection_pause_at_safepoint(). That would 
>> remove the need
>> for EvacuationInfo::reset().
>>
>>
>> Maybe it's better to inline the call to
>> _evacuation_info.register_collectionset_regions() inside to
>> g1_policy()->finalize_cset()?
>>
>>          g1_policy()->finalize_cset(target_pause_time_ms);
>> _evacuation_info.register_collectionset_regions(g1_policy()->cset_region_length()); 
>>
>>
>>
>> gcTrace.hpp
>>
>> Why did you move G1OldTracer to within "#ifndef SERIALGC" the other 
>> tracer are
>> not guarded by this unless they have to (as G1NewTracer)?
>>
>> I think it is better to forward declare the EvacuationInfo class than 
>> including
>> gc_implementation/g1/evacuationInfo.hpp since you only use 
>> EvacuationInfo*. (If
>> you decide to keep the include you need to move it in under #ifndef 
>> SERIALGC.)
>>
>>
>> evacuationInfo.hpp:
>> I don't think it needs this include:
>> #include "gc_implementation/g1/heapRegion.hpp"
>>
>> Why do you use += for all the setters? For the cases where you do 
>> accumulate
>> (like in G1CollectedHeap::free_collection_set()) I think it would be 
>> better to
>> accumulate in a local variable and then just call register once with 
>> the total.
>>
>> Bengt
>>
>>
>> On 4/10/13 1:39 AM, Jesper Wilhelmsson wrote:
>>> Hi,
>>>
>>> Got some internal feedback (thanks Bengt!) and here is a new version:
>>>
>>> http://cr.openjdk.java.net/~jwilhelm/8009032/hotspot-webrev/
>>>
>>> Thanks,
>>> /Jesper
>>>
>>>
>>>
>>> Jesper Wilhelmsson skrev 4/4/13 4:04 PM:
>>>> Hi,
>>>>
>>>> I'm looking for a couple of reviews for this change.
>>>>
>>>> This is an tracing event with information about G1 
>>>> evacuation/compaction. It
>>>> contains the number of regions in from-space and to-space, how much 
>>>> data that
>>>> was used before and after, the number of bytes copied during the
>>>> evacuation/compaction, and how many regions that was completely 
>>>> freed during an
>>>> evacuation.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8009032/webrev/
>>>>
>>>>
>>>> The patch also contains a bunch of fixed typos in various comments 
>>>> nearby the
>>>> relevant changes. These may add some noise to the webrev, if you 
>>>> prefer to look
>>>> at the webrev without these changes a separate webrev is available 
>>>> here:
>>>>
>>>> Webrev without typo fixes:
>>>> http://cr.openjdk.java.net/~jwilhelm/8009032/webrev-no_typo_fix/
>>>>
>>>>
>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8009032
>>>>
>>>> Thanks,
>>>> /Jesper
>>