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 ->
7145569-test-apply/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 ->
7145569-test-apply/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 ->
7145569-test-apply/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 <
~/public_html/webrevs/7145569/webrev.all/g1-nmethod-scanning.patch
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"
OK.
> - comment for strong_code_roots_do(): "Applied" -> "Apply" or "Applies"?
OK.
> - 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
formed");
assert(check_heap_region_claim_values(HeapRegion::InitialClaimValue),
"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:
public:
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"
OK.
> - line 6560, 6587: assert(!hr->isHumongous(), "nmethod oop in
> *h*umongous?");, not "numongous".
OK.
> - 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);
Thanks,
JohnC
More information about the hotspot-gc-dev
mailing list