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
