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