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

[Mikael Gerdin]

Hi Roman,

On 2017-06-30 18:32, Roman Kennke wrote:
> 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.

I think refactoring the code to not expect a singleton CollectedHeap 
instance is a bit too much.
Perhaps there is another way to share common code between Serial and CMS 
but that might require a bit more thought.

/Mikael

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