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

[Roman Kennke]

That sounds like a good idea. I'll give it a try.

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