<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hi Jesper,<br>
    <br>
    I've only looked at the first set of changes. I'll look at the
    second set later tonight or tomorrow.<br>
    <br>
    <div class="moz-cite-prefix">On 3/20/2013 4:46 PM, Jesper
      Wilhelmsson wrote:<br>
    </div>
    <blockquote cite="mid:514A4A73.6080704@oracle.com" type="cite">Hi,
      <br>
      <br>
      I'm looking for a couple of reviews for this change.
      <br>
      <br>
      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.
      <br>
      <br>
      I have split the change into two parts:
      <br>
      <br>
      <br>
      1) JDK-8009992: Prepare tracing of promotion failed for
      integration of evacuation failed
      <br>
      <br>
      Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jwilhelm/8009992/webrev/">http://cr.openjdk.java.net/~jwilhelm/8009992/webrev/</a>
      <br>
      <br>
      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.
      <br>
    </blockquote>
    <br>
    Looks good. Just one nit:<br>
    <br>
    <code></code>
    <u>src/share/vm/gc_implementation/shared/gcTraceSend.cpp</u><br>
    <blockquote type="cite">
      <pre><span class="new">  95 static TraceStructCopyFailed to_trace_struct(const CopyFailedInfo&amp; cf_info) {</span>
<span class="new">  96   TraceStructCopyFailed failed_info;</span>
<span class="new">  97   failed_info.set_objectCount(cf_info.failed_count());</span>
<span class="new">  98   failed_info.set_firstSize(cf_info.first_size());</span>
<span class="new">  99   failed_info.set_smallestSize(cf_info.smallest_size());</span>
<span class="new"> 100   failed_info.set_totalSize(cf_info.total_size());</span>
<span class="new"> 101   failed_info.set_thread(cf_info.thread()-&gt;thread_id());</span>
<span class="new"> 102   return failed_info;</span>
<span class="new"> 103 }</span>
<span class="new"> 104 </span>
 105 void YoungGCTracer::send_promotion_failed_event(const PromotionFailedInfo&amp; pf_info) const {
 106   EventPromotionFailed e;
 107   if (e.should_commit()) {
 108     e.set_gcId(_shared_gc_info.id());
<span class="changed"> 109     e.set_data(to_trace_struct(pf_info));</span>
 110     e.commit();
 111   }
 112 }</pre>
    </blockquote>
    <br>
    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.<br>
    <br>
    to_trace_struct(e.data_addr(), pf_info);<br>
    <br>
    where to_trace_struct() is:<br>
    <br>
    void to_trace_struct(TraceStructCopyFailed* dest_data, const
    CopyFailedInfo&amp; cf_info) {<br>
    &nbsp; dest_data-&gt;set_objectCount(cf_info.failed_count());<br>
    &nbsp; ...<br>
    }<br>
    <br>
    Other than that. It looks OK to me. Now on to the other one. <br>
    <br>
    JohnC<br>
    <br>
  </body>
</html>