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). 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. - 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 Thanks, Thomas
