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

Roman Kennke rkennke at redhat.com
Mon Jul 10 14:10:59 UTC 2017


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.

>
> - 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.
> - 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
>
Removed all of them.

> - 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)?
Yes, it is. Just a habit of mine to make it more readable (to me). Fixed it.
> - one extra newline here:
>   92 bool CMSHeap::create_cms_collector() {
>   93
Fixed.
> - 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;
>
Ok, good point. Fixed.

> - maybe skip the // success comment here:
>   111   return true;  // success
That was probably pre-existing too. Should be thankful that it did not
say return true; // return true :-P

> - 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   }
>
Duh. Fixed.

> - 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.
Great catch! I love it when refactoring leads to simplifications...
Fixed.
> - do you really need #ifdef ASSERT around check_gen_kinds?
>
No, not really.

> - 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,
>     };
>   };
>
Good idea. Done.

> Have you though about vmStructs.cpp, does it need any changes?
No. I don't really know what needs to go in there. I added:

  declare_constant(CollectedHeap::CMSHeap)                                \

just so that it's there next to the other heap types. Not sure what else
may be needed, if anything?

http://cr.openjdk.java.net/~rkennke/8179387/webrev.05/
<http://cr.openjdk.java.net/%7Erkennke/8179387/webrev.05/>

Better now?

Roman




More information about the hotspot-gc-dev mailing list