RFR: 8008916 G1: Evacuation failed tracing event

[Jesper Wilhelmsson]

Bengt, all,

A new webrev is now available:

http://cr.openjdk.java.net/~jwilhelm/8008916/webrev/

The main difference is that now we have an array in G1CollectedHeap that keeps 
track the EvacuationFailedInfo-objects between the different parallel parts of 
G1 so that each thread only sends a single EvacuationFailed event in the end.

To achieve this some logic was added to find the correct info object in the 
array since the threads does not have the same queue number each time.

Thanks,
/Jesper


Bengt Rutisson skrev 28/3/13 10:20 PM:
>
> Jesper,
>
> This looks much better!
>
> Have you tested it to see that you get a maximum of one event per GC thread and
> GC? It looks to me like you risk getting multiple events sent for each GC thread
> every GC.
>
> You send the events in the G1ParScanThreadState destructor. I am not that
> familiar with G1ParScanThreadState but it looks to me like we create new
> G1ParScanThreadState instances for the different phases of a GC. Here are the
> places where we create new instances of G1ParScanThreadState:
>
> G1CollectedHeap::process_discovered_references()
> G1ParPreserveCMReferentsTask::work()
> G1ParTask::work()
> G1STWRefProcTaskProxy::work()
>
> So, it looks to me like we risk getting 4 events sent per thread and GC.
>
>
> Some minor comments:
>
> In G1CollectedHeap::handle_evacuation_failure_par() you pass the ef_info on to
> G1CollectedHeap::handle_evacuation_failure_common(). But the ef_info is now
> thread local so you don't have to take the lock to use it. One way to simplify
> this would be to do "ef_info->register_copy_failure(old->size());" in
> G1CollectedHeap::handle_evacuation_failure_par() just before "if
> (_evac_failure_closure != cl)" instead. Then you don't have to pass it on to
> handle_evacuation_failure_common().
>
> (As a side note, I don't think setting _evacuation_failed to true has to be
> protected by the lock either. It is a shared variable but all writes to it from
> different threads will be to set it to true, so I think racing on it is benign.
> But you have it in the same place as before, which might be good.)
>
> In G1ParCopyClosure::copy_to_survivor_space() you are now picking out two thing
> from the _par_scan_state and passing them on to
> G1CollectedHeap::handle_evacuation_failure_par(). Have you considered passing a
> reference to _par_scan_state instead and let handle_evacuation_failure_par()
> extract the data it needs?
>
> If you move the implementation of the destructor of G1ParScanThreadState in to
> the cpp file you don't have to add the include for gcTrace.hpp to
> g1CollectedHeap.hpp.
>
> Why did you decide to remove set_evactuation_failed()?
>
> Finally, you have several quite unrelated spelling corrections in this change.
> This makes it a bit hard to review. I'm not sure I think it is worth the extra
> review time to get those fixed.
>
> Bengt
>
>
> On 3/28/13 7:50 PM, Jesper Wilhelmsson wrote:
>> 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
>