CRR (M): 7127706: G1: re-enable survivors during the initial-mark pause

[Bengt Rutisson]

Tony,

Overall this looks really good. Thanks for fixing it.

Some comments:

First, a general question regarding naming and logging. We now talk 
about "snapshot" a lot. It is a pretty good name, but maybe it needs 
some more context to be understandable in the code and the GC log. I 
don't have any really good names, but maybe "survivor_snapshot" or 
"initial_mark_snapshot"?

concurrentMark.inline.hpp

   if (hr == NULL) {
     hr = _g1h->heap_region_containing_raw(addr);
     // Given that we're looking for a region that contains an object
     // header it's impossible to get back a HC region.
     assert(!hr->continuesHumongous(), "sanity");
   } else {
     assert(hr->is_in(addr), "pre-condition");
   }

The first assert should probably hold even for regions that are passed 
in to grayRoot() right? So, maybe something like:

   if (hr == NULL) {
     hr = _g1h->heap_region_containing_raw(addr);
   } else {
     assert(hr->is_in(addr), "pre-condition");
   }
   // Given that we need a region that contains an object
   // header it's impossible for it to be a HC region.
   assert(!hr->continuesHumongous(), "sanity");


concurrentMarkThread.cpp

ConcurrentMarkThread::run()

Why do we do the explicit time/date stamping?

           gclog_or_tty->date_stamp(PrintGCDateStamps);
           gclog_or_tty->stamp(PrintGCTimeStamps);
           gclog_or_tty->print_cr("[GC concurrent-snapshot-scan-start]");

why is it not enough with the normal -XX:+PrintGCTimeStamps information? 
This is probably correct since I see this pattern in other places. But I 
would like to understand why we do it.


g1CollectedHeap.cpp:

G1CollectedHeap::do_collection()

Is it worth logging how long we had to wait in 
_cm->snapshot_regions()->wait_until_scan_finished(), the same way that 
we do in G1CollectedHeap::do_collection_pause_at_safepoint()?


Finally, just some food for thought. Could this be generalized to more 
roots? I mean take a snapshot and scan it concurrently.

Bengt



On 2012-01-18 00:48, Tony Printezis wrote:
> Hi all,
>
> Can I have a couple of code reviews for this change that re-enables 
> the use of survivor regions during the initial-mark pause?
>
> http://cr.openjdk.java.net/~tonyp/7127706/webrev.0/
>
> From the CR:
>
> We could scan the survivors as we're copying them, however this will 
> require more work during the initial-mark GCs (and in particular: 
> special-case code in the fast path).
>
> A better approach is to let the concurrent marking threads scan the 
> survivors and mark everything reachable from them a) before any more 
> concurrent marking work is done (so that we can just mark the objects, 
> without needing to push them on a stack, and let the "finger" 
> algorithm discover them) and b) before the next GC starts (since, if 
> we copy them, we won't know which of the new survivors are the ones we 
> need to scan).
>
> This approach has the advantage that it does not require any extra 
> work during the initial-mark GCs and all the work is done by the 
> concurrent marking threads. However, it has the disadvantage that the 
> survivor scanning might hold up the next GC. In most cases this should 
> not be an issue as GCs take place at a reasonably low rate. If it does 
> become a problem we could consider the following:
>
> - like when the GC locker is active, try to extend the eden to give a 
> bit more time to the marking threads to finish scanning the survivors
> - instead of waiting for the marking threads, a GC can take over and 
> finish up scanning the remaining survivors (typically, we have more GC 
> threads than marking threads, so the overhead will be reduced)
> - if we supported region pinning, we could pin all the regions that 
> were not scanned by the time the GC started so that the marking 
> threads can resume scanning them after the GC completes
>
> Implementation notes:
>
> I introduced the concept of a "snapshot regions" in the ConcurrentMark 
> which is a set of regions that need to be scanned at the start of a 
> concurrent cycle. Currently, these can only be survivors but maybe we 
> can use the same concept for something else in the future.
>
> Tony
>
>