RFR: 8008916 G1: Evacuation failed tracing event

[Bengt Rutisson]

Hi Jesper,

A couple of comments:

As I said before I don't like the lazy allocation of the 
EvacutaionFailedInfo objects. I would prefer to allocate all the objects 
the G1CollectedHeap constructor. This would make the code much simpler. 
I know you are worried about false sharing, but I don't think the 
current approach reduces the risk of false sharing. If it really is a 
problem in the evacuation failed case (which I am not convinced of) you 
should pad the EvacuationFailedInfo objects instead.

I would prefer that we always look the EvacutaionFailedInfo objects up 
in the G1CollectedHeap::_evacuation_failed_info_array rather than to 
include them in the G1ParScanThreadState.

So, do "_g1h->_evacuation_failed_info_array[_queue_num]" instead of 
"_par_scan_state->evacuation_failed_info()" in 
G1CollectedHeap::handle_evacuation_failure_par(). That means that you 
don't have to change G1ParScanThreadState much at all. No extra fields 
and no changes to the method declarations.

I think this is more future proof also, since the next step is probably 
to add a separate data structure to allow us to look up 
EvacuationFailedInfo objects based on the thread id.

I would also prefer that EvacuationFailedInfo inherits from CHeapObj 
instead of VALUE_OBJ_CLASS_SPEC. That way you can use the normal new 
operator instead of placement new.

Bengt


On 4/16/13 12:01 PM, Jesper Wilhelmsson wrote:
> Hi,
>
> After several different versions and realizations about the worker 
> threads in G1, here is now a new version of the evacuation failed event.
>
> To include the correct thread with each event has turned out to 
> require more work and footprint overhead than what can be motivated at 
> this point, and since any other solution would be confusing rather 
> than informative, we have decided not to include the thread 
> information in the evacuation failed event for now.
>
> The thread information was never a requirement in the first place, we 
> just threw it in there in the promotion failed event because it was 
> easily available in the parallel collectors and someone might be able 
> to use it for debugging at some point.
>
> An RFE [1] has been created to remind us to look at this again for G1.
>
> The updated webrev is here:
> http://cr.openjdk.java.net/~jwilhelm/8008916/webrev/
>
> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008916
>
> [1]: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8012217
>
> Thanks!
> /Jesper
>
> Jesper Wilhelmsson skrev 28/3/13 7:50 PM:
>> Hi,
>>
>> A new version of the evacuation failed event for G1. Now, each thread 
>> sends an
>> event with the info it has collected during the collection in the 
>> same way as
>> the other collectors do with promotion failed events.
>>
>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8008916/webrev.2/
>>
>> Thanks,
>> /Jesper