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

[Roman Kennke]

Am 03.07.2017 um 09:35 schrieb 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.

Yeah, definitely. I hit another difficulty: pretty much the same issues
that I'm having with GenCollectedHeap/CMSHeap/CollectedHeap now show up
with Generation and its subclasses..

How about we push the original patch that I've posted, and work from
there? In fact, I *have* found some little things I would change (some
more if (UseConcMarkSweepGC) branches in GenCollectedHeap that I have
overlooked in my first pass...)

Roman

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