RFR: Adding evacuation failed tracing event

[Erik Helin]

John,

On 03/22/2013 12:16 AM, John Cuthbertson wrote:
> 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().)

No, it won't. This is because e.set_data() takes a const reference to a 
TraceStructCopy. See [0] for more information about how this works.

The following code shows an example:

#include <cstdio>

   class Foo {
     int _x;
    public:
     Foo(int x) : _x(x) {
       printf("Constructing a new Foo\n");
     }
     Foo(const Foo &f) {
       _x = f.x();
       printf("Copying a Foo\n");
     }
     ~Foo() {
       printf("Destructing a Foo\n");
     }

     int x() const { return _x; }
   };

   Foo create_foo() {
     Foo f(17);
     return f;
   }

   void print_foo(const Foo &foo) {
     printf("Foo %d\n", foo.x());
   }

   int main() {
     print_foo(create_foo());

     return 0;
   }

Running:

   g++ -O0 foo.cpp -o foo ; ./foo

Results in:

   Constructing a new Foo
   Foo 17
   Destructing a Foo

Thanks,
Erik

[0]: 
http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/

> 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
>
>