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

[Roman Kennke]

(I included hotspot-runtime-dev and serviceability-dev to review
vmStructs.cpp changes. see below)

Hi Erik,

>> 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.
>
> Because you are accessing CMSCollcetor in:
>
>  99   NOT_PRODUCT(
>  100     virtual size_t skip_header_HeapWords() { return
> CMSCollector::skip_header_HeapWords(); }
>  101   )
>
> and CMSCollector is declared in concurrentMarkSweepGeneration.hpp. An
> alternative would of course be to just declare skip_header_HeapWords()
> in cmsHeap.hpp and define skip_header_HeapWords in cmsHeap.cpp, then
> you only need to include concurrentMarkSweeoGeneration.hpp in
> cmsHeap.cpp.
Ah ok, I've missed that one. Added it now.

>>> 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'.
>
> It is perfectly fine to have private virtual methods in C++ (see for
> example
> https://stackoverflow.com/questions/2170688/private-virtual-method-in-c).
> A virtual function only needs to be protected if a "child class" needs
> to access the function in the "parent class". For both gc_prologue and
> gc_epilogue, this is the case, which is why they have to be
> 'protected' in GenCollectedHeap. But, no class is going to derive from
> CMSHeap, so they can be private in CMSHeap.
Cool. Learned something new :-) It actually makes sense.

I've moved all 3 methods into the private block in CMSHeap. I left them
virtual (because of missing override), and I also left them in protected
in GenCollectedHeap (prologue/epilogue because we need to,
skip_header_HeapWords() to not confuse readers.)
 
>>> 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?
>
> Honestly, I don't know, that is why I asked if someone else with more
> knowledge in this area can comment. Have you tried building and using
> the SA agent with your change? You can also ask around on
> hotspot-rt-dev and or serviceability-dev.
I haven't tried building SA. I poked around
hotspot/src/jdk.hotspot.agent and I think it should be ok. Can somebody
who knows about it confirm this?

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

Roman