RFR: 8067499: G1SATBCardTableModRefBS should not inherit from CardTableModRefBSForCTRS

[Mikael Gerdin]

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

Nice cleanup, I recently stumbled across this too and was annoyed by how 
G1 was messing around with GenRemSet for no good reason.

I suspect that part of the historic reason for this is that the 
GenRemSet (as the comment in SharedHeap suggests) was used to keep track 
of pointers from the perm gen into the G1 heap, and G1 had to iterate 
over dirty perm gen cards to update those pointers when doing an evacuation.
Now that the perm gen is dead and buried we can get rid of this code!


Some further suggested cleanups:
* Move CollectorPolicy::create_rem_set to GenCollectorPolicy and change 
the call site to use the GenCollectedHeap::gen_policy() to get the 
GenCollectorPolicy instance to avoid the casting.
* Fold CardTableRS into GenRemSet (it's the only implementor of the 
GenRemSet interface) and as far as I know we have no plans of adding a 
completely new kind of barrier-/remset variant to the GenCollectedHeap 
based collectors.

/Mikael

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