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

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

- why are gc_prologue and gc_epilogue protected in CMSHeap? Can't they
   be private in CMSHeap?

- there are two `private:` blocks, please use only one `private:`
   block.

- one extra newline here:
   32 class CMSHeap : public GenCollectedHeap {
   33

- one extra newline here:
   46
   47

cmsHeap.cpp:
- one extra newline here:
   36 CMSHeap::CMSHeap(GenCollectorPolicy *policy) : 
GenCollectedHeap(policy) {
   37

- one extra newline here:
   65
   66

- do you need to use `this` here?
   87   this->GenCollectedHeap::print_on_error(st);

   Isn't it enough to just GenCollectedHeap::print_on_error(st)?

- one extra newline here:
   92 bool CMSHeap::create_cms_collector() {
   93

- this is pre-existing, but since we are copying code, do we want to
   clean it up?
   104   if (collector == NULL || !collector->completed_initialization()) {
   105     if (collector) {
   106       delete collector;  // Be nice in embedded situation
   107     }
   108     vm_shutdown_during_initialization("Could not create CMS 
collector");
   109     return false;
   110   }

   The collector == NULL check is not needed here. CMSCollector derives
   from CHeapObj and CHeapObj::operator new will by default do
   vm_exit_out_of_memory if the returned memory is NULL. The check can
   just be:

   if (!collector->completed_initialization()) {
     vm_shutdown_during_initialization("Could not create CMS collector");
     return false;
   }
   return true;

- maybe skip the // success comment here:
   111   return true;  // success

- is it possible to end up in CMSHeap::should_do_concurrent_full_gc()
   if we are not using CMS? As in:
   123 bool CMSHeap::should_do_concurrent_full_gc(GCCause::Cause cause) {
   124   if (!UseConcMarkSweepGC) {
   125     return false;
   126   }

- one extra newline here:
   135
   136

genCollectedHeap.hpp:
- I don't think you have to make _skip_header_HeapWords protected.
   Instead I think we can skip_header_HeapWords() virtual, make it
   return 0 in GenCollectedHeap and return
   CMSCollector::skip_header_HeapWords in CMSHeap and just remove the _
   skip_header_HeapWords variable.

- do you really need #ifdef ASSERT around check_gen_kinds?

- can you make GCH_strong_roots_tasks a protected enum in
   GenCollectedHeap? As in
   class GenCollectedHeap : public CollectedHeap {
   protected:
     enum StrongRootTasks {
       GCH_PS_Universe_oops_do,
     };
   };

Have you though about vmStructs.cpp, does it need any changes?

Thanks,
Erik

> Roman
>