Simplifying the BarrierSet hierarchy

[Kim Barrett]

> On Nov 22, 2016, at 6:22 AM, Roman Kennke <rkennke at redhat.com> wrote:
> 
> While working on the GC interface prototype, I made a sketch of the
> current BarrierSet hierarchy:
> 
> x  BarrierSet
> x   ModRefBarrierSet
> x     CardTableModRefBS
> x      G1SATBCardTableModRefBS
> g       G1SATBCardTableLoggingModRefBS
> p      CardTableExtension
> c      CardTableModRefBSForCTRS
> 
> x - not used anywhere
> g - used by G1
> p - used by parallel
> c - used by CMS
> 
> I've got a couple of questions:
> 
> - Is that correct and complete? Especially for CMS, is it only using
> CardTableModRefBSForCTRS and is it the only user of that class? I am
> asking, because I want to do the oop_store() refactoring as outlined in
> the GC interface JEP, and if it's only used for CMS, I can move the
> special oop_store() handling to there.

Have you looked at
https://bugs.openjdk.java.net/browse/JDK-8163897
oop_store has unnecessary memory barriers

At a quick look, there may be some similar wierdness with
klass_oop_store.

> - Is there any reason against simplifying that hierarchy? In
> particular, I would collapse unused classes like this:
> 
> x  BarrierSet
> x   CardTableModRefBS
> g    G1SATBCardTableLoggingModRefBS
> p    CardTableExtension
> c    CardTableModRefBSForCTRS
> 
> I.e. merge ModRef with CardTableModRef and G1SATBCardTable with
> G1SATBCardTableLogging.
> 
> Any opinions?

I did some cleaning up in this are a while ago, e.g.

8067499: G1SATBCardTableModRefBS should not inherit from CardTableModRefBSForCTRS
8130931: Refactor CardTableModRefBS[ForCTRS]
(and I think there were some similar cleanups by others)

and had intended to do more, but got distracted by other
problems. There is more to be done here, such as

CardTableModRefBS::is_card_dirty - used in one place, in G1
CardTableModRefBS::is_card_clean - unused
CardTableModRefBS::mark_card_dirty - unused
CardTableModRefBS::align_to_card_boundary - unused
CardTableModRefBS::dirty_card_iterate - used only by CMS
CardTableModRefBS::dirty_card_range_after_reset - used only by CMS
CardTableModRefBS::is_aligned - confusing synonym for is_card_aligned

There is still some stuff not relevant to G1 in CardTableModRefBS,
such as

CardTableModRefBS::resize_covered_region - G1 implementation is shouldNotReachHere
CardTableModRefBS::find_covering_region_by_base - only in resize_covered_region
CardTableModRefBS::committed_unique_to_self - only in resize_covered_region
CardTableModRefBS::largest_prev_committed_end - only in resize_covered_region
CardTableModRefBS::find_covering_region_containing - used only by CMS
and possibly some data members

CardTableModRefBS::initialize - G1 has a different initialization protocol

This suggests there ought to be another class in the hierarchy for
those operations.

And I'm pretty sure CardTableModRefBSForCTRS is not "clean", in that
it contains CMS-specific stuff that isn't needed by SerialGC,
suggesting another potential split.