RFR: 8009032 - Implement evacuation info event

[Jesper Wilhelmsson]

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
>