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

Roman Kennke rkennke at redhat.com
Mon Jul 3 13:08:22 UTC 2017


In fact, my original plan was to also factor out the serial specific
stuff into a new subclass. This means, everything that is truly shared
between SerialHeap and CMSHeap would remain in GenCollectedHeap (for
now), and everything else would move down to the specific subclasses.
Then we can see what remains shared, and what is GC specific, and go
from there. What do you think?

Roman

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




More information about the hotspot-gc-dev mailing list