RFR: 8179387: Factor out CMS specific code from GenCollectedHeap into its own subclass
Erik Helin
erik.helin at oracle.com
Fri Jul 14 10:21:54 UTC 2017
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;
>>
>> - 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:
86 protected:
87 void gc_prologue(bool full);
88 void gc_epilogue(bool full);
89
90 protected:
91 // Accessor for memory state verification support
92 NOT_PRODUCT(
93 virtual size_t skip_header_HeapWords() { return
CMSCollector::skip_header_HeapWords(); }
94 )
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.
>> - 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.
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;
Btw, this code looks really fishy :) The CMSCollector is created with
new but the pointer (collector) is never stored anywhere. It works,
becaues the constructor for CMSCollector sets a static variable in
ConcurrentMarkSweepGeneration, but it isn't exactly beautiful :) Don't
change this now, I just wanted to point it out, since the code looks a
bit mysterious.
>> - 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?
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?
Thanks,
Erik
> 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