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

[Thomas Schatzl]

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.

- I cannot really comment on the completeness of compiler changes.

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.

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.

heapRegion.hpp

- push_strong_code_root vs. add_strong_code_root

- 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.

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.

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

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.

- about use of another data structure for the per-region strong roots
list: I agree, but depending on the typical size of these arrays.

- 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.

- 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?

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.

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

- 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.

- 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.

- 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. 

- 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.

- 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

- 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).

- 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?

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).

- 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.

Thomas