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

John Cuthbertson john.cuthbertson at oracle.com
Tue Jun 18 16:28:45 UTC 2013

Hi Thomas,

Thanks for looking at the changes. Replies inline....

On 6/14/2013 3:15 AM, Thomas Schatzl wrote:
> Hi John,
>    I had some initial look at the code, first comments (mostly about
> typos, comments) below:
> On Thu, 2013-06-13 at 15:30 -0700, John Cuthbertson 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.
> - the patch does not seem to apply to latest hotspot-gc repository here,
> getting a few failed hunks.

The webrev.all/g1-nmethod-scanning.patch applies just fine for me:

cairnapple{jcuthber}:79> new_workspace -out 7145569-test-apply
## http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot -> 7145569-test-apply
requesting all changes
adding changesets
adding manifests
adding file changes
added 4801 changesets with 31115 changes to 4795 files
updating to branch default
3953 files updated, 0 files merged, 0 files removed, 0 files unresolved
## http://closedjdk.us.oracle.com/hsx/hotspot-gc/hotspot/src/closed -> 
requesting all changes
adding changesets
adding manifests
adding file changes
added 936 changesets with 2343 changes to 481 files
updating to branch default
333 files updated, 0 files merged, 0 files removed, 0 files unresolved
## http://closedjdk.us.oracle.com/hsx/hotspot-gc/hotspot/test/closed -> 
requesting all changes
adding changesets
adding manifests
adding file changes
added 570 changesets with 1365 changes to 624 files
updating to branch default
555 files updated, 0 files merged, 0 files removed, 0 files unresolved
## http://closedjdk.us.oracle.com/hsx/hotspot-gc/hotspot/make/closed -> 
requesting all changes
adding changesets
adding manifests
adding file changes
added 108 changesets with 112 changes to 15 files
updating to branch default
15 files updated, 0 files merged, 0 files removed, 0 files unresolved

cairnapple{jcuthber}:80> cd 7145569-test-apply
cairnapple{jcuthber}:81> gpatch -p1 < 
patching file src/share/vm/c1/c1_Runtime1.cpp
patching file src/share/vm/code/nmethod.cpp
patching file src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
patching file src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp
patching file src/share/vm/gc_implementation/g1/g1GCPhaseTimes.cpp
patching file src/share/vm/gc_implementation/g1/g1GCPhaseTimes.hpp
patching file src/share/vm/gc_implementation/g1/g1RemSet.cpp
patching file src/share/vm/gc_implementation/g1/g1RemSet.hpp
patching file src/share/vm/gc_implementation/g1/g1_globals.hpp
patching file src/share/vm/gc_implementation/g1/heapRegion.cpp
patching file src/share/vm/gc_implementation/g1/heapRegion.hpp
patching file src/share/vm/gc_interface/collectedHeap.hpp

> g1_globals.hpp:
> - I am not a particular fan of the "If true, ..." prefix for globals
> descriptions, it seems redundant. But this is really imo.

OK. Changed the wording to "Verify the code root lists attached to each 
heap region."

> nmethod.cpp:
> - Universe::heap()->register_nmethod(this); seems to be always paired
> with CodeCache::add_scavenge_root_nmethod(this). Maybe it is useful to
> put the nmethod registration in that method.

I would rather not have "registration" dependent upon not being on the 
scavengable nmethod list. I can see in the future that registration may 
actually perform adding to the scavengable list but I would like to keep 
them separate for now.

> heapRegion.hpp
> - push_strong_code_root vs. add_strong_code_root

OK. Renamed. I had a "pop" operation that wasn't being used so I removed it.

> - comment for migrate_strong_code_roots(): "... they *ppoint* into"


> - comment for strong_code_roots_do(): "Applied" -> "Apply" or "Applies"?


> - comment for verify_strong_code_roots(): "... include at *lease*...";
> The method does not return the number of failures as the failures
> parameter is a bool.

OK. Fixed the typo and removed the statement about the return value.

> g1RemSet.hpp
> - worker_i should be an uint; there has been a pre-hs24 (7121618) issue
> that changed worker_i variables across all collectors from int to uint.
> Recently the use of "int" has crept in into g1 code again. If it causes
> trouble (because int is used in g1), leave it - there is a new CR
> (8016302) to fix that at once.

I would prefer to leave this to the other CR. The changes really started 
to ripple through the rest of the code.

> - comment for oops_into_collection_set_do(): " .. this param will be
> *ingored*"

OK. Some people get bent out of shape about fixing typos in comments 
that aren't really being touched. If I get complaints I'll send 'em your 
way. :)

> heapRegion.cpp:
> - In HeapRegion::hr_clear(), it may be interesting to not only clear the
> code root list, but deallocate it. Depending on the size of this list
> (do you have numbers?) this may lead to excessive memory use of free
> regions' HeapRegions. I.e. there have been too many issues recently
> about G1 taking too much space by default :) In the same direction,
> maybe the default size of 10 could be lowered. Eg. with 60k+ region
> heaps, this will add 4M+ to the heap at startup.
> Then again, this could be done in another cr when fixing the general
> issue.

I'm OK with this. The down side is that 
GrowableArray::clear_and_deallocate() is private so I have to delete the 
pointer and allocate anew. Is this OK?

> - at the end of the file there is this change that (forward?) declares
> the "template class GrowableArray<nmethod*>;". As it's visible only in
> that cpp file, and afterwards there is no code, it seems superfluous.

OK. I needed this to avoid linker errors when I declared the field as a 
growable array of CodeBlob*. It's seems I don't need it for nmethods.

> - in remove_strong_code_root() the code tries to remove multiple entries
> of a given nmethod. How is it possible to get multiple duplicate
> entries, assuming that push_strong_code_root() already does not push an
> nmethod if it is already in the list?

Good point. I don't think it should. Previously I was only checking the 
top most element in the list for duplication. Is it better to defend 
against it or just guarantee it?

> g1CollectedHeap.cpp:
> - in the line "G1VerifyCodeRootBlobClosure
> blobsCl(&codeRootsCl, /*do_marking=*/ false);" I think common style is
> to do the comment with spaces around and no equals sign, e.g. /*
> do_marking */, and after the actual parameter.

OK. Changed:
     G1VerifyCodeRootBlobClosure blobsCl(&codeRootsCl, false /* 
do_marking */);

> - line 3884, the text for the assert has wrong indentation (compared to
> the assert above)

OK. Changed:

     assert(check_young_list_well_formed(), "young list should be well 
            "sanity check");

> - in the comment in line 4990, maybe explicitly mention that this is
> about regions in the collection set once. E.g.
> "... that point into a region *in the collection set* are scanned when
> we scan ...". Not absolutely necessary I think.

OK. Changed comment to:

      // Don't scan the scavengable methods in the code cache as part
       // of strong root scanning. The code roots that point into a
       // region in the collection set are scanned when we scan the 
region's RSet

> - after trying to fix mentioned patch import errors, I get
> warnings/errors when compiling this file. It seems that in
> RegisterNMethodOopClosure/UnregisterNMethodOopClosure ::do_oop_work() a
> decode_heap_oop_not_null() is missing.
> I.e. change
>    template <class T> void do_oop_work(T* p) {
>      T heap_oop = oopDesc::load_heap_oop(p);
>      if (!oopDesc::is_null(heap_oop)) {
>        HeapRegion* hr = _g1h->heap_region_containing(heap_oop);
> to
>    template <class T> void do_oop_work(T* p) {
>      T heap_oop = oopDesc::load_heap_oop(p);
>      if (!oopDesc::is_null(heap_oop)) {
>        oop obj = oopDesc::decode_heap_oop_not_null(heap_oop); <!----
>        HeapRegion* hr = _g1h->heap_region_containing(obj);
> but that may be due to the merge errors experienced above.

OK. Done. Not sure why you get the merge errors though.

> - in G1CollectedHeap::[un]register_nmethod() there is this argument
> checking code at the beginning:
>    assert(nm != NULL, "sanity");
>    if (nm == NULL) return;
> This seems confusing - either passing NULL is an error (I suspect so -
> make it a guarantee()?) or not imo.

OK. It comes from when I was using CodeBlobs rather than nmethods. It 
used to be:

assert(cb != NULL, "sanity");
nmethod* nm = cb->as_nmethod_or_null();
if (nm == NULL) return.

I changed the above to a guarantee than nm was not null.

> - maybe explicitly state that the MarkStrongCodeRootCodeBlobClosure is
> the closure for regions not in the collection set. While the first
> sentence implies that (and at the location of the use of this class it
> is described), it took me a while to get that.

OK. Changed the comment to:

// Mark all the code roots that point into regions *not* in the
// collection set.
// Note we do not want to use a "marking" CodeBlobToOopClosure while
// walking the the code roots lists of regions not in the collection
// set. Suppose we have an nmethod (M) that points to objects in two
// separate regions - one in the collection set (R1) and one not (R2).
// Using a "marking" CodeBlobToOopClosure here would result in "marking"
// nmethod M when walking the code roots for R1. When we come to scan
// the code roots for R2, we would see that M is already marked and it
// would be skipped and the objects in R2 that are referenced from M
// would not be evacuated.

class MarkStrongCodeRootCodeBlobClosure: public CodeBlobClosure {

> - I find it odd that comments for MarkStrongCodeRootCodeBlobClosure is
> after the "class MarkStrongCodeRootCodeBlobClosure : ... " line. In all
> other cases in this file it is before the declaration. Maybe this
> comment is for the containing MarkStrongCodeRootOopClosure, but then it
> would be better to move it away from the
> MarkStrongCodeRootCodeBlobClosure declaration (imo).
> - same comment, maybe add some (to me clarifying) words: "We don't want
> to get in the following situation*:* if *concurrent* marking sees an
> nmethod whose oops span a couple of regions (one in the coll...". The
> conditional of the if does not seem to have a dependent clause. At least
> I (not a native speaker) have a hard time parsing this sentence as this
> lengthy comment indicates.
> - same comment: "... he code root list for region*s* not in the " and
> "When we come to scan the code root list for the region in the
> collection *set*" I think

See reworded comment above.

> - in MarkStrongCodeRootOopClosure::do_oop_work() the comment about _hr
> ".. which is assumed to be not in the collection set" could be made
> stronger by asserting that somewhere (i.e. add a "!
> _hr->is_in_collection_set()" assert).

OK. Added the assert in the constructor:

     MarkStrongCodeRootOopClosure(ConcurrentMark* cm, HeapRegion* hr, 
uint worker_id) :
       _cm(cm), _hr(hr), _worker_id(worker_id) {
       assert(!_hr->in_collection_set(), "sanity");

> - line 5103: "partipate"->"participate"


> - line 6560, 6587:  assert(!hr->isHumongous(), "nmethod oop in
> *h*umongous?");, not "numongous".


> - line 6562: in the comment, it might be useful (or in the documentation
> of push_strong_root()) to specify that push_strong_code_root()
> eliminates duplicates already. Or does this comment is about that a
> single nmethod is pushed multiple times into different region's nmethod
> remembered sets?

OK. Changed the comment to:

       // HeapRegion::add_strong_code_root() avoids adding duplicate
       // entries but having duplicates is  OK since we "mark" nmethods
       // as visited when we scan the strong code root lists during the GC.

It was left over when I was just checking the top most entry (which 
eliminates a large amount of duplication).

> g1CollectedHeap.hpp
> - the comment for migrate_strong_code_roots() seems to not take into
> account collection failures when the nmethods stay in the same region.
> But maybe to-space includes that (I really don't know).

OK. Changed the comment to:

   // Migrate the nmethods in the code root lists of the regions
   // in the collection set to regions in to-space. In the event
   // of an evacuation failure, nmethods that reference objects
   // that were not successfullly evacuated are not migrated.
   void migrate_strong_code_roots();

> - mark_strong_code_roots() only seems to mark parts of the heap not in
> the collection set, not the entire heap, contrary to the comment.

OK. Changed comment to:

   // During an initial mark pause, mark all the code roots that
   // point into regions *not* in the collection set.
   void mark_strong_code_roots(uint worker_id);



More information about the hotspot-gc-dev mailing list