RFR: 8009032 - Implement evacuation info event

[Bengt Rutisson]

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