RFR (XL): 8151386: Extract card live data out of G1ConcurrentMark

[Thomas Schatzl]

Hi Kim,

On Mon, 2016-04-04 at 19:38 -0400, Kim Barrett wrote:
> > On Mar 29, 2016, at 9:38 AM, Thomas Schatzl wrote:
> > http://cr.openjdk.java.net/~tschatzl/8151386/webrev.3/ (full)
> > http://cropenjdk.java.net/~tschatzl/8151386/webrev.2_to_3 (diff)
>
> Looks good.
> Just a few minor comments and a couple of questions.  If doing >
 anything about any of these ends up having fannout, feel free to
> defer to another RFR.  (I'm thinking of things like removing an 
> unused parameter, which might percolate backward for some 
> distance.)  I don't think I need a new webrev for any of these.

Thanks for your review. Since all of your comments did not amount to
many changes in total, I fixed them all as requested.

I put an updated (final) webrev at
http://cr.openjdk.java.net/~tschatzl/8151386/webrev.4/ (full)

Passed jprt.

(Sorry, no diffs. I messed up, had already folded my stack of changes
down to the final change before I noticed that I hadn't incorporated
your last comment about verify_card_live_data_is_clear() yet).

I will finally push these two changes (i.e. +8077144, I wanted to wait)
tomorrow if there are no further comments or other obstructions.

Thanks,
  Thomas


> ---------------------------------------------------------------------
> --------- 
> src/share/vm/gc/g1/g1ConcurrentMark.cpp
>  929 void G1ConcurrentMark::scanRootRegion(HeapRegion* hr, uint
> worker_id) {
> 
> With the change to G1RootRegionScanClosure, it looks like the
> worker_id argument for scanRootRegion is no longer used.
> 
> ---------------------------------------------------------------------
> --------- 
> src/share/vm/gc/g1/g1ConcurrentMark.cpp
> 2039     GCTraceTime(Debug, gc)("Clear Bitmap");
> 
> Could the description string be more descriptive, e.g. which bitmap?
> 
> Similarly here:
> src/share/vm/gc/g1/g1CollectedHeap.cpp
> 1427         GCTraceTime(Debug, gc)("Clear Bitmap for Verification");
> 
> ---------------------------------------------------------------------
> --------- 
> src/share/vm/gc/g1/g1ConcurrentMark.hpp
>  651   // Verify live data.
>  652   void verify_live_data();
> 
> That description comment is even more vacuous than the one it
> replaced.
> 
> ---------------------------------------------------------------------
> --------- 
> src/share/vm/gc/g1/g1ConcurrentMark.hpp
>  855   // Precondition: obj is below region's NTAMS.
> 
> Why was this comment removed?
> 
> Oh, right, removal of the region argument left "region's" in the
> comment dangling, which I noted in review of the earlier change that
> removed that argument.  But removing the comment was not what I
> suggested.  I think the comment should be retained but changed to
> 
>   // Precondition: obj is below its region's NTAMS.
> 
> ---------------------------------------------------------------------
> --------- 
> src/share/vm/gc/g1/g1ConcurrentMark.inline.hpp
>   32 #include "logging/log.hpp"
> 
> Why is this being included here?  I'm not finding any uses of logging
> in this file.
> 
> ---------------------------------------------------------------------
> --------- 
> src/share/vm/gc/g1/g1ConcurrentMark.inline.hpp
>  263 inline bool G1ConcurrentMark::do_yield_check(uint worker_id) {
> 
> The worker_id is unused.  I think with these changes there are now no
> callers that provide a value for that argument, with all call sites
> now using the default value of 0.
> 
> ---------------------------------------------------------------------
> --------- 
> src/share/vm/gc/g1/g1RemSet.cpp
>  605 #ifndef PRODUCT
>  606 void G1RemSet::verify_card_live_data_is_clear() {
> 
> I *think* the call chain that reaches here is DEBUG-only, which is
> good since extra verification is for DEBUG builds, not optimize or
> product builds.  So I think that should be #ifdef ASSERT, and
> similarly update the declaration in the header, and similarly
> G1CardLiveData::verify_is_clear.
> 
> ---------------------------------------------------------------------
> ---------
>