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

Fri Jul 14 12:00:14 UTC 2017

On 07/14/2017 01:22 PM, Roman Kennke wrote:
> Hi Erik,
> 
>> On 07/10/2017 04:10 PM, Roman Kennke wrote:
>>> Am 10.07.2017 um 15:13 schrieb Erik Helin:
>>>> On 07/07/2017 03:21 PM, Roman Kennke wrote:
>>>>> Am 07.07.2017 um 14:35 schrieb Erik Helin:
>>>>>> On 07/06/2017 06:23 PM, Roman Kennke wrote:
>>>>>>>>>> Ok to push this?
>>>>>>>>
>>>>>>>> I just realized that your change doesn't build on Windows since you
>>>>>>>> didn't #include "precompiled.hpp" in cmsHeap.cpp. MSVC is really
>>>>>>>> picky
>>>>>>>> about that.
>>>>>>>> /Mikael
>>>>>>>
>>>>>>> Uhhh.
>>>>>>> Ok, here's revision #3 with precompiled added in:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~rkennke/8179387/webrev.03/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.03/>
>>>>>>
>>>>>> Hi Roman,
>>>>>>
>>>>>> I just started looking :) I think GenCollectedHeap::gc_prologue and
>>>>>> GenCollectedHeap::gc_epilogue should be virtual, and
>>>>>> always_do_update_barrier = UseConcMarkSweepGC moved down
>>>>>> CMSHeap::gc_epilogue.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Yes, I have seen that. My original plan was to leave it as is
>>>>> because I
>>>>> know that Erik Ö. is working on a big barrier set refactoring that
>>>>> would
>>>>> remove this code anyway. However, it doesn't really matter, here's the
>>>>> cleaned up patch:
>>>>>
>>>>> http://cr.openjdk.java.net/~rkennke/8179387/webrev.04/
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.04/>
>>>>
>>>> A few comments:
>>>>
>>>> cmsHeap.hpp:
>>>> - you are missing quite a few #includes, but it works since
>>>>     genCollectedHeap.hpp #includes a whole lot of stuff. Not
>>>> necessary to
>>>>     fix now, because the "missing #include" will start to pop up when
>>>>     someone tries to break apart GenCollectedHeap into smaller pieces.
>>> Right.
>>> I always try to minimize includes, especially in header files (they are
>>> bound to proliferate later anyway). In addition to that, if a class is
>>> only referenced as pointer, I avoid includes and use forward class
>>> definition instead.
>>
>> I think that we in general try to include what is needed, not what
>> only what makes the code compile (header guards will of course ensure
>> that the header files are only parsed once). So in cmsHeap.hpp, at
>> least I would have added:
>>
>>    #include "gc/cms/concurrentMarkSweepGeneration.hpp"
>>    #include "gc/shared/collectedHeap.hpp"
>>    #include "gc/shared/gcCause.hpp"
>>
>> and forward declared:
>>
>>    class CLDClosure;
>>    class OopsInGenClosure;
>>    class outputStream;
>>    class StrongRootsScope;
>>    class ThreadClosure;
> 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.

>>>>
>>>> - why are gc_prologue and gc_epilogue protected in CMSHeap? Can't they
>>>>     be private in CMSHeap?
>>> They are virtual and protected in GenCollectedHeap and called by
>>> GenCollectedHeap. Makes sense to also make them protected in CMSHeap? Or
>>> am I missing something?
>>>
>>>> - there are two `private:` blocks, please use only one `private:`
>>>>     block.
>>>>
>>> Fixed.
>>
>> And now there is two `protected:` blocks, immediately after each other:
>>
> Duh. Fixed.
> 
>> 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.

skip_header_HeapWords needs to be virtual, since classes inheriting from 
GenCollectedHeap might want to change its behavior. However, no class 
inheriting from GenCollectedHeap (only CMSHeap so far) needs to call 
GenCollectedHeap::skip_header_HeapWords, so it can actually be private 
virtual in GenCollectedHeap. But, in order to not confuse readers, it 
might better to keep it protected virtual in GenCollectedHeap. There is 
no reason to have skip_header_HeapWords protected in CMSHeap though, 
there it should be declared private (and potentially virtual, since 
override comes first in C++11).

>> Sorry, reading the code again it is obvious that create_cms_collector
>> never can return false. It either returns true or calls
>> vm_shutdown_during_initialization (which will not return). So, I would
>> just make create_cms_collctor void, the if branch below is dead code:
>>
>>    51   if (!success) return JNI_ENOMEM;
>>
> Right! Very good catch! Changed that.
> 
>> Btw, this code looks really fishy :)
> Err, yep. I'll make a note somewhere (in bugs.o.j.n) to fix that later.
>> 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.

Thanks,
Erik

> http://cr.openjdk.java.net/~rkennke/8179387/webrev.06.diff/
> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.06.diff/>
> http://cr.openjdk.java.net/~rkennke/8179387/webrev.06/
> <http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.06/>
> 
> Thanks for reviewing!
> Roman
> 