RFR: 8009032 - Implement evacuation info event

[John Cuthbertson]

Hi Jesper,

This looks good.

I was mistaken about G1CollectorPolicy::_bytes_copied_during_gc *not* 
including the occupancy of the retained old GC allocation region. It does.

Near the end of the pause we call release_gc_alloc_regions(). In that 
routine we release the current _old_gc_alloc_region:

> 4278   // If we have an old GC alloc region to release, we'll save it in
> 4279   // _retained_old_gc_alloc_region. If we don't
> 4280   // _retained_old_gc_alloc_region will become NULL. This is what we
> 4281   // want either way so no reason to check explicitly for either
> 4282   // condition.
> 4283   _retained_old_gc_alloc_region = _old_gc_alloc_region.release();

When we release this region:

> HeapRegion* G1AllocRegion::release() {
>   trace("releasing");
>   HeapRegion* alloc_region = _alloc_region;
>   retire(false /* fill_up */);
>   assert(_alloc_region == _dummy_region,
>          ar_ext_msg(this, "post-condition of retire()"));
>   _alloc_region = NULL;
>   trace("released");
>   return (alloc_region == _dummy_region) ? NULL : alloc_region;
> }

we retire the region via:

G1AllocRegion::release() -> G1AllocRegion::retire() -> 
OldGCAllocRegion::reire_region() -> 
G1CollectedHeap::retire_gc_alloc_region() -> 
G1CollectorPolicy::record_bytes_copied_during_gc()

which updates G1CollectorPolicy::_bytes_copied_during_gc.

Therefore, I don't think you need you need to have the 
_alloc_regions_used_before field in the EvacuationInfo struct or the 
related material. That should simplify a couple of things. Many apologies.

On a stylistic note, my preference would have been to have some kind of 
units in the field names, e.g. num_cset_regions (or cset_region_num), 
used_bytes_before, etc. But I can live with the current names.

JohnC

On 4/10/2013 1: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
>>