RFR: 8008916 G1: Evacuation failed tracing event

[Bengt Rutisson]

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