RFR: 8008916 G1: Evacuation failed tracing event
bengt.rutisson at oracle.com
Thu Mar 28 14:20:39 PDT 2013
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
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
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
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.
On 3/28/13 7:50 PM, Jesper Wilhelmsson wrote:
> 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/
More information about the hotspot-gc-dev