RFR: 8067499: G1SATBCardTableModRefBS should not inherit from CardTableModRefBSForCTRS

[Bengt Rutisson]

Hi Kim,

On 2014-12-17 07:14, Kim Barrett wrote:
> Please review this change to G1SATBCardTableModRefBS to derive from
> CardTableModRefBS rather than CardTableModRefBSForCTRS.
>
> Note: I will need a sponsor for this change.
>
> That inheritance change runs afoul of SharedHeap providing remembered set
> support.  That seems inappropriate, since the provided remembered set support
> is oriented toward generational collection (with incremental update barriers),
> but generations show up in GenCollectedHeap, which is derived from SharedHeap.
>
> To address that, the rem_set() member (and associated data member) were moved
> from SharedHeap down to GenCollectedHeap.  This permitted some cleanup in the
> G1 code, eliminating references to the otherwise vestigal global remembered
> set object.  (G1 remembered sets are a different sort of creature.)
>
> A different way of addressing the remembered set location would have been to
> change G1CollectedHeap to derive from CollectedHeap rather than SharedHeap,
> and that's a change that has been suggested as a technical debt cleanup.  That
> change would have required similar changes to G1 code, plus a number of other
> changes.  This move of remembered set support can be viewed as a step toward
> changing the heap hierarchy.
>
> It may be that the G1 barrier set shouldn't derive from CardTableModRefBS
> either; CardTableModRefBS provides a lot of stuff that G1 doesn't use or need,
> some of which has to be overridden or called for compatibility.
> Alternatively, it might be that some things presently in CardTableModRefBS
> should be moved down to CardTableModRefBSForCTRS.  But again, that larger
> refactoring would require more changes, and this move of remembered set
> support and shift of the G1 barrier set base class can be viewed as a step
> toward that larger refactoring.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8067499
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8067499/webrev

Great cleanup!

One thought about this code in cardTableRS.cpp:

   41 #if INCLUDE_ALL_GCS
   42   guarantee(!UseG1GC, "sanity");
   43 #endif

I don't think we need the INCLUDE_ALL_GCS guard here since the UseG1GC 
is always available. Also, I think I would prefer to check for what we 
actually expect instead of what we don't expect. This is what we are 
expecting, right?

guarantee(UseSerialGC | UseConcMarkSweepGC, "santiy");

Thanks,
Bengt

>
> Testing:
> Local jtreg of hotspot/test/{runtime,gc}.
> JPRT
> refworkload with G1, CMS, and ParOld collectors.
>