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

[Roman Kennke]

I came across one problem using this approach: We will have 2 instances
of CollectedHeap around, where there's usually only 1, and some code
expects only 1. For example, in CollectedHeap constructor, we create new
PerfData variables, and we now create them 2x, which leads to an assert
being thrown. I suspect there is more code like that.

I will attempt to refactor this a little more, maybe it's not that bad,
but it's probably not worth spending too much time on it.

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