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

[coleen.phillimore at oracle.com]

On 7/19/17 6:42 AM, Roman Kennke wrote:
> Am 17.07.2017 um 16:42 schrieb coleen.phillimore at oracle.com:
>>
>> On 7/17/17 8:07 AM, Roman Kennke wrote:
>>> (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/>
>> http://cr.openjdk.java.net/~rkennke/8179387/webrev.07/src/share/vm/runtime/vmStructs.cpp.udiff.html
>>
>>
>> I'm not sure why you added this because it's not in the agent java
>> files.  SA builds as a part of the whole build and there are some
>> basic tests in hotspot/test/serviceability/sa  .  If those run, you're
>> probably fine.  The SA has some copied code for CMS but it appears to
>> be minimal and the team is working now on deciding what functionality
>> to provide, so I suggest not adding code that might not be used.
> It's just a constant/enum, and I added it because all the other possible
> values of that enum are also there. I can remove it though:
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8179387/webrev.08.diff/
> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.08.diff/>

Thanks, this is good.  I don't know enough about the rest of the change 
to be a reviewer, but I think you have your reviews.
Coleen

> Full:
> http://cr.openjdk.java.net/~rkennke/8179387/webrev.08/
> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.08/>
>
> Better?
>
> Roman
>