RFR: 8179387: Factor out CMS specific code from GenCollectedHeap into its own subclass

[Erik Helin]

Hi Roman,

thanks for putting this patch together, it is a great step forward! One
thung that (in my mind) would improve it even further is if we embed a
GenCollectedHeap in CMSHeap and then make CMSHeap inherit directly from
CollectedHeap.

With this solution, the definition of CMSHeap would look like something
along the lines of:

class CMSHeap : public CollectedHeap {
  WorkGang* _wg;
  GenCollectedHeap _gch;

 public:
  CMSHeap(GenCollectorPolicy* policy) :
    _wg(new WorkGang("GC Thread", ParallelGCThreads, true, true),
    _gch(policy) {
    _wg->initialize_workers();
  }

  // a bunch of "facade" methods
  virtual bool supports_tlab_allocation() const {
    return _gch->supports_tlab_allocation();
  }

  virtual size_t tlab_capacity(Thread* t) const {
    return _gch->tlab_capacity(t);
  }
};

With this approach, you would have to implement a bunch of "facade"
methods that just delegates to _gch, such as the methods
supports_tlab_allocation and tlab_capacity above. There are two reasons
why I prefer this approach:
1. In the end we want CMSHeap to inherit from CollectedHeap anyway :)
2. It makes it very clear which methods we gradually have to
   re-implement in CMSHeap to eventually get rid of the _gch field (the
   end goal). This is much harder to see if CMSHeap inherits from
   GenCollectedHeap (see more below).

The second point will most likely cause some initial problems with
`protected` code in GenCollectedHeap. For example, as you noticed when
creating this patch, CMSHeap make use of a few `protected` fields and
methods from GenCollectedHeap, most notably:
- _process_strong_tasks
- process_roots()
- process_string_table_roots()

It would be much better (IMO) to share this code via composition rather
than inheritance. In this particular case, I would prefer to create a
class StrongRootsProcessor that encapsulates the root processing logic.
Then GenCollectedHeap and CMSHeap can both contain an instance of
StrongRootsProcessor.

What do you think of this approach? Do you have some spare cycles to try
this approach out?

Thanks,
Erik

On 06/02/2017 10:55 AM, Roman Kennke wrote:
> Take this patch. It #ifdef ASSERT's a call to check_gen_kinds() that is
> only present in debug builds.
> 
> 
> http://cr.openjdk.java.net/~rkennke/8179387/webrev.01/
> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.01/>
> 
> Roman
> 
> Am 01.06.2017 um 22:50 schrieb Roman Kennke:
>> What $SUBJECT says.
>>
>> I went over genCollectedHeap.[hpp|cpp] and moved everything that I could
>> find that is CMS-only into a new CMSHeap class.
>>
>> http://cr.openjdk.java.net/~rkennke/8179387/webrev.00/
>> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.00/>
>>
>> It is possible that I overlooked something there. There may be code in
>> there that doesn't shout "CMS" at me, but is still intrinsically CMS stuff.
>>
>> Also not that I have not removed that little part:
>>
>>   always_do_update_barrier = UseConcMarkSweepGC;
>>
>> because I expect it to go away with Erik Ö's big refactoring.
>>
>> What do you think?
>>
>> Testing: hotspot_gc, specjvm, some little apps with -XX:+UseConcMarkSweepGC
>>
>> Roman
>>
>