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

Mikael Gerdin mikael.gerdin at oracle.com
Mon Jul 3 07:35:26 UTC 2017

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.


> 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