RFR: JDK-8055845 - Add trace event for promoted objects

[Staffan Friberg]

Hi,

Uploaded a new webrev, with the changes from your comments here and from 
Bengt's email.
http://cr.openjdk.java.net/~sfriberg/8055845/webrev.04

See inline for comments.


On 09/15/2014 05:57 AM, Thomas Schatzl wrote:
> Hi,
>
> On Fri, 2014-09-05 at 15:20 -0700, Staffan Friberg wrote:
>> Hi,
>>
>> I have uploaded a new webrev here,
>> cr.openjdk.java.net/~sfriberg/8055845/webrev.03
>>
>> It contains several changes
>>
>>      - Split event into two events (PromoteObjectInNewPLAB,
>> PromoteObjectOutsidePLAB)
>>      - Moved events to "vm/gc/detailed/PromoteObject..."
>>      - Supporting ParNew+CMS and ParNew+SerialOld tenuring
>>           - Not sure if the way I do it with passing the ParNewTracer
>> is the best solution, please let me know if you have an idea how to
>> improve it
>>      - Simplified the G1 code to avoid sending age and having a single
>> call site
>>      - Fixed so that the generated event has size information in bytes
>> rather than words
>>
>> Thanks for offline comments and suggestions from Dmitry and Thomas.
>    - in G1CollectedHeap::par_allocate_during_gc() I still think it is
> required to do the !old->is_forwarded() check before retrieving the
> old->age(), and so that there cannot be a reload of the mark header
> between those.
>
> Also, if you look at oopDesc::age(), the first assert checks whether it
> is not forwarded. Between the atomic claim by installing the forward
> pointer and this reading of the age this might happen, so the assert may
> trigger.
>
> So the change should either read the mark oop first (using a volatile
> read), then do the is_forwarded() check and the retrieval of the age
> value on that mark oop, do the even processing after the claiming of the
> object (forward pointer installing) as suggested once, or (least
> favourable to me) pass the markOop down.
>
> The latter messes up the method signatures, and in any case (when using
> option one or three) this code is slightly racy as we might report too
> many events as another thread might have claimed the object. (Parallel
> Scavenge has the same issue, in addition to the possibility of sending
> two events as Bengt describes).
Good catch with the assert had missed that, will read it similar to how 
it is done G1ParScanThreadState::copy_to_survivor_space instead, which 
avoids the assert.

I think the best way would be to read the age and then check 
is_forwarded, like now but without the assert issue. If is_forwarded is 
false the read age will be OK to use, if it is true it might be a valid 
age or overwritten by the forward, but I won't be using it so that is 
fine. It would still have the risk of not actually doing the copying 
later, but at least all the data about the object should be correct at 
this point.


   if (result != NULL && _gc_tracer_stw->should_report_promotion_event()) {
     markOop m = old->mark();
     uint age = m->has_displaced_mark_helper() ? 
m->displaced_mark_helper()->age()
                                               : m->age();
     // Check if object has already been promoted by another thread
     if (!old->is_forwarded()) {
       _gc_tracer_stw->report_promotion_event(old, age,
               !heap_region_containing_raw(result)->is_young(), word_size);
     }
   }


I don't follow why you would like the age to be read after the 
is_forwarded. Trusting the age is my main concern here and reading it 
prior to the is_forward check I think would solve that. Could you expand 
on this further?


>
> Please document the possibility of the race, and the workaround in these
> locations.
>
>    - another source of inaccuracy is that at the end of GC, G1 will make
> the very last PLAB available for allocation in the next GC. And it may
> do additional allocations to fill up the region (if there is not enough
> useful space at the end of the allocation region), or fill up the old
> gen allocation to the next card to avoid races in the next GC (see
> G1CollectedHeap::release_gc_alloc_regions() and the release() methods of
> the Survivor/OldGCAllocRegion classes.
>
> This, that JFR might get slightly too many events (or too few), should
> be documented somewhere, probably in JFR/event documentation. At least
> the sum of these allocations should not be used as the number of copied
> bytes.
I think the total number of allocated PLABs should be OK as I don't 
think we ever return a PLAB. But as you say we might count a single 
object twice or more if it end up requiring a new PLAB in multiple threads.

Added the following to the description of the two events "Due to 
promotion being done in parallel an object might be reported multiple 
times as the GC threads race to copy all objects."

>    - also maybe add a comment about the purpose of the "old" parameters
> passed around. It is not obvious that a method that allocates a range of
> bytes needs the value of the old memory block. Except for CMS, where it
> is already used, the other non-G1 methods lack this documentation too.
>
>    - please align the parameters in all calls to
> gc_tracer->report_promotion_event().
>
>    - there is a superfluous space at the end of the line in
> generation.hpp:326
Done

Cheers,
Staffan