RFR: Adding evacuation failed tracing event

[John Cuthbertson]

Hi Jesper,

I've only looked at the first set of changes. I'll look at the second 
set later tonight or tomorrow.

On 3/20/2013 4:46 PM, Jesper Wilhelmsson wrote:
> Hi,
>
> I'm looking for a couple of reviews for this change.
>
> The change is to add evacuation failed tracing events to G1. Since the 
> evacuation failed is very similar to a regular promotion failed in 
> other young GCs, these events share most of the code.
>
> I have split the change into two parts:
>
>
> 1) JDK-8009992: Prepare tracing of promotion failed for integration of 
> evacuation failed
>
> Webrev: http://cr.openjdk.java.net/~jwilhelm/8009992/webrev/
>
> This part refactors the promotion failed code to use a generic copy 
> failed class which is used as the base for the promotion failed handling.

Looks good. Just one nit:

|| _src/share/vm/gc_implementation/shared/gcTraceSend.cpp_
>    95 static TraceStructCopyFailed to_trace_struct(const CopyFailedInfo& cf_info) {
>    96   TraceStructCopyFailed failed_info;
>    97   failed_info.set_objectCount(cf_info.failed_count());
>    98   failed_info.set_firstSize(cf_info.first_size());
>    99   failed_info.set_smallestSize(cf_info.smallest_size());
>   100   failed_info.set_totalSize(cf_info.total_size());
>   101   failed_info.set_thread(cf_info.thread()->thread_id());
>   102   return failed_info;
>   103 }
>   104
>   105 void YoungGCTracer::send_promotion_failed_event(const PromotionFailedInfo& pf_info) const {
>   106   EventPromotionFailed e;
>   107   if (e.should_commit()) {
>   108     e.set_gcId(_shared_gc_info.id());
>   109     e.set_data(to_trace_struct(pf_info));
>   110     e.commit();
>   111   }
>   112 }

Won't this result in executing the copy constructors of 
TraceStructCopyFailed multiple times? (One in the return from 
to_trace_struct() and one in set_data().) Would it not be more efficient 
to return the address of the struct in the EventPromotionFailed, i.e.

to_trace_struct(e.data_addr(), pf_info);

where to_trace_struct() is:

void to_trace_struct(TraceStructCopyFailed* dest_data, const 
CopyFailedInfo& cf_info) {
   dest_data->set_objectCount(cf_info.failed_count());
   ...
}

Other than that. It looks OK to me. Now on to the other one.

JohnC

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130321/80b406b6/attachment.htm>