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

[Kim Barrett]

> On Mar 29, 2016, at 9:38 AM, Thomas Schatzl <thomas.schatzl at oracle.com> 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.

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

------------------------------------------------------------------------------