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

[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.

Thanks,
Coleen

>
> Roman
>