RFR: 8009032 - Implement evacuation info event

[Jesper Wilhelmsson]

Bengt,

Thanks for reviewing!

Bengt Rutisson skrev 11/4/13 10:22 AM:
>
> 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.

Fixed.

>
> A couple of minor things in trace.xml:
>
> I think the field name collectionSetRegions could be cSetRegions to be
> consistent with cSetUsedBefore and cSetUsedAfter.

Fixed.

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

This was intentional. Most of our events don't have any description, but I added 
it to the cSet and allocRegions fields since they caused confusion and needed to 
be explained.

Thanks,
/Jesper

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