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

[Roman Kennke]

Hi Erik,

> On 07/10/2017 04:10 PM, Roman Kennke wrote:
>> Am 10.07.2017 um 15:13 schrieb Erik Helin:
>>> On 07/07/2017 03:21 PM, Roman Kennke wrote:
>>>> Am 07.07.2017 um 14:35 schrieb Erik Helin:
>>>>> On 07/06/2017 06:23 PM, Roman Kennke wrote:
>>>>>>>>> Ok to push this?
>>>>>>>
>>>>>>> I just realized that your change doesn't build on Windows since you
>>>>>>> didn't #include "precompiled.hpp" in cmsHeap.cpp. MSVC is really
>>>>>>> picky
>>>>>>> about that.
>>>>>>> /Mikael
>>>>>>
>>>>>> Uhhh.
>>>>>> Ok, here's revision #3 with precompiled added in:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~rkennke/8179387/webrev.03/
>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.03/>
>>>>>
>>>>> Hi Roman,
>>>>>
>>>>> I just started looking :) I think GenCollectedHeap::gc_prologue and
>>>>> GenCollectedHeap::gc_epilogue should be virtual, and
>>>>> always_do_update_barrier = UseConcMarkSweepGC moved down
>>>>> CMSHeap::gc_epilogue.
>>>>>
>>>>> What do you think?
>>>>
>>>> Yes, I have seen that. My original plan was to leave it as is
>>>> because I
>>>> know that Erik Ö. is working on a big barrier set refactoring that
>>>> would
>>>> remove this code anyway. However, it doesn't really matter, here's the
>>>> cleaned up patch:
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/8179387/webrev.04/
>>>> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.04/>
>>>
>>> A few comments:
>>>
>>> cmsHeap.hpp:
>>> - you are missing quite a few #includes, but it works since
>>>    genCollectedHeap.hpp #includes a whole lot of stuff. Not
>>> necessary to
>>>    fix now, because the "missing #include" will start to pop up when
>>>    someone tries to break apart GenCollectedHeap into smaller pieces.
>> Right.
>> I always try to minimize includes, especially in header files (they are
>> bound to proliferate later anyway). In addition to that, if a class is
>> only referenced as pointer, I avoid includes and use forward class
>> definition instead.
>
> I think that we in general try to include what is needed, not what
> only what makes the code compile (header guards will of course ensure
> that the header files are only parsed once). So in cmsHeap.hpp, at
> least I would have added:
>
>   #include "gc/cms/concurrentMarkSweepGeneration.hpp"
>   #include "gc/shared/collectedHeap.hpp"
>   #include "gc/shared/gcCause.hpp"
>
> and forward declared:
>
>   class CLDClosure;
>   class OopsInGenClosure;
>   class outputStream;
>   class StrongRootsScope;
>   class ThreadClosure;
Ok, added those and some more that I found. Not sure why we'd need
#include "gc/cms/concurrentMarkSweepGeneration.hpp" ? Left that out for now.

>>>
>>> - why are gc_prologue and gc_epilogue protected in CMSHeap? Can't they
>>>    be private in CMSHeap?
>> They are virtual and protected in GenCollectedHeap and called by
>> GenCollectedHeap. Makes sense to also make them protected in CMSHeap? Or
>> am I missing something?
>>
>>> - there are two `private:` blocks, please use only one `private:`
>>>    block.
>>>
>> Fixed.
>
> And now there is two `protected:` blocks, immediately after each other:
>
Duh. Fixed.

> IMO, I would just make the three functions above private. I know they
> are protected in GenCollectedHeap, but it should be fine to have them
> private in CMSHeap. Having them protected signals, at least to me,
> that this class could be considered as a base class (protected to me
> reads "this can be accessed by classes inheriting from this class),
> and we don't want any class to inherit from CMSHeap.

How can they be called from the superclass if they are private in the
subclass? Would that work in C++?

protected (to me) means visibility between super and subclasses. If I'd
want to signal that I intend that to be overridden, I'd say 'virtual'.

> Sorry, reading the code again it is obvious that create_cms_collector
> never can return false. It either returns true or calls
> vm_shutdown_during_initialization (which will not return). So, I would
> just make create_cms_collctor void, the if branch below is dead code:
>
>   51   if (!success) return JNI_ENOMEM;
>
Right! Very good catch! Changed that.

> Btw, this code looks really fishy :)
Err, yep. I'll make a note somewhere (in bugs.o.j.n) to fix that later.
> This is for the serviceability agent. You will have to poke around in
> hotspot/src/jdk.hotspot.agent and see how GenCollectedHeap is used.
> Unfortunately I'm not that familiar with the agent, perhaps someone
> else can chime in here?

Considering that the remaining references to GenCollectedHeap in
vmStructs.cpp don't look like related to CMSHeap, I'd argue that what I
did is all that's needed for now. Do you agree?

http://cr.openjdk.java.net/~rkennke/8179387/webrev.06.diff/
<http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.06.diff/>
http://cr.openjdk.java.net/~rkennke/8179387/webrev.06/
<http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.06/>

Thanks for reviewing!
Roman