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

[Roman Kennke]

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/>
Full:
http://cr.openjdk.java.net/~rkennke/8179387/webrev.08/
<http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.08/>

Better?

Roman