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

[Roman Kennke]

Am 19.07.2017 um 14:29 schrieb 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/>

Ping? I believe this never actually went in. Is there anything missing? 
Do you want me to re-base it onto the single-repo and post another webrev?

Roman