RFR: 8067499: G1SATBCardTableModRefBS should not inherit from CardTableModRefBSForCTRS

[Kim Barrett]

Thanks for your review Mikael.

On Dec 17, 2014, at 4:20 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
> 
> On 2014-12-17 07:14, Kim Barrett wrote:
>> Please review this change to G1SATBCardTableModRefBS to derive from
>> CardTableModRefBS rather than CardTableModRefBSForCTRS.
>> 
>> […]
>> 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!

Yes, that’s how it looks to me too.

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

Those are on my radar.

As part of that we should be able to eliminate a pre-existing unchecked down-cast in the OopsInGenClosure constructor.

src/share/vm/memory/genOopClosures.inline.hpp [line 47 changed for this review]
  47     GenRemSet* rs = GenCollectedHeap::heap()->rem_set();
  48     _rs = (CardTableRS*)rs;

Such a lovely cast, completely breaking the abstraction!