RFR(M/L): 7145569: G1: optimize nmethods scanning

[Christian Thalinger]

The compiler related part looks good to me.

-- Chris

On Jun 13, 2013, at 3:30 PM, John Cuthbertson <john.cuthbertson at oracle.com> wrote:

> Hi Everyone,
> 
> Can I have a few volunteers look over the changes for this CR - ideally I would like at least one person from the compiler team and one from the GC team.
> 
> Summary:
> For some workloads with G1, the scanning of the Code cache cache can take quite a long time. In fact there have been cases where the scanning of the code cache is the dominator of the pause and the other worker threads sit an spin in the termination code while the worker that claimed the code cache scanning task chugs along. Part of the reason is that the list of nmethods with scavengable oops is a single root task. Another part, specifically affecting G1, is that the presence of an nmethod in the list depends on the definition of is_scavengable. For the other collectors, when the oops in an nmethod are promoted to the old generation that nmethod ceases to be scavengable and is pruned from the scavengable list. For G1, because we can collect "old" data during a mixed evacuation pause we can't prune the scavengable nmethod list.
> 
> The changes are my attempt at a solution outlined by Igor Veresov.
> 
> I have added a list of nmethods to each HeapRegion. That list is populated by nmethods that contain roots that point into that heap region. The lists are populated when an nmethod is created (or in the case of C1 when it is patched).
> 
> During an evacuation pause we skip scanning the entire code cache and instead scan the oops in each nmethod attached to a region when we scan that regions RSet. Near the end of the pause we have migrate the nmethods attached to regions in the collection to regions in to-space. During an initial mark pause, we walk the nmethods attached to all heap regions (other than those in the collection set - they're marked when their nmethod lists are scanned). During a full GC we empty the nmethod list attached to each region and then rebuild it - similar to what happens to the RSets.
> 
> To verify the integrity of these nmethod lists I've extended the heap verification. When scanning the code cache we check that an nmethod is attached to heap regions that it points into. When verifying the heap regions we ensure that the nmethods attached to a region contain at least one pointer into that region. This additional verification can add some time so I recently put it under control of a diagnostic flag (testing was performed with it enabled).
> 
> As part of these changes I moved some code around (specifically the verification code) in g1CollectedHeap.[ch]pp and heapRegion.[ch]pp. The purely clean up changes can be found at: http://cr.openjdk.java.net/~johnc/7145569/webrev.0.code-movement/
> 
> The webrev containing just the functionality can be found at: http://cr.openjdk.java.net/~johnc/7145569/webrev.1.optimize-nmethod-scanning/
> 
> The whole shebang can be found at: http://cr.openjdk.java.net/~johnc/7145569/webrev.all
> 
> Testing:
> GC test suite with C1, C2, and Tiered on x64 and sparc (Xmixed and Xcomp) - with and without verification (including the extra verification);
> a few jprt runs
> Kitchensink (4 hour runs) with C1, C2, Tiered on x64 (Xmixed and Xcomp) - with and without verification (including the extra verification).
> 
> Future enhancements:
> * Currently I'm using a growable array for the nmethod lists but in the long term I want to move to a more suitable data structure. Perhaps a specialized version of Stack.
> * Currently I migrate nmethods from regions in the collection set to regions in to-space in a separate phase. Ideally this should be done when we're finished scanning a region's RSet. When we do this, migration will be performed in parallel and two threads could be pushing to a to-space regions nmethod list at the same time - we will need an "atomic" push operation. When the compilers push an nmethod, the do it while holding the CodeCache_lock so such an operation is, currently, unnecessary.
> 
> Thanks,
> 
> JohnC
> 
>