RFR: 8189171: Move GC argument processing into GC specific classes

[Roman Kennke]

Hi Erik,

thank you for your suggestions and changes.

I like it: if you look at webrev.00 you will notice that my original 
proposal had both the static stuff and the virtual stuff in the same 
class too. So from there it's mostly the renaming exercise :-)

So let me just put your changes up for review (again), if you don't mind:

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/ 
<http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.03/>

Incremental over webrev.02:
http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/ 
<http://cr.openjdk.java.net/%7Eeosterlund/8189171/webrev.02_03/>

So I suppose we can count it by reviewed ok by you?

Then it requires one more review to go in, right?

Thanks, Roman


> Hi Roman,
>
> Definitely looks better. So the static GCArgumentProcessor has a 
> singleton GCCollectedHeapFactory that is created during 
> GCArgumentProcessor::initialize().
> That makes sense to me, but I think it can be even better.
>
> First off - sorry for bikeshedding a bit here.
>
> To save you from having to update the code due to my bikeshedding, I 
> have prepared a patch to show you what I meant, and you can tell me 
> whether you agree or not.
>
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.03/
>
> Incremental over yours:
> http://cr.openjdk.java.net/~eosterlund/8189171/webrev.02_03/
>
> My changes are the following:
>
> I collapsed the all-static GCArgumentProcessor and the 
> CollectedHeapFactory into the same class called GCArguments (the 
> natural GC helper for Arguments). It's a singleton object that deals 
> with GC arguments stuff.
>
> GCArguments has some static functions to create_instance(), creating 
> the singleton GCArguments object, and after you have created the 
> singleton instance, you access it through GCArguments::instance(), and 
> it will point to a GC-specific GCArguments, e.g. G1Arguments, 
> CMSArguments, etc.
>
> The main reason why I think it's better to have a GC-specific 
> GCArguments class compared to a CollectedHeapFactory class, is that I 
> would like to eventually move all arguments processing into the 
> GCArguments class, regardless of where (before or after CollectedHeap 
> exists) in the boot strapping process. For example, today some 
> argument stuff is dumped in the arguments.cpp file, and other stuff is 
> dumped in CollectorPolicy::initialize_flags which is called later when 
> the heap exists where it seemingly reads values out from flags, into 
> internal variables, then changes them to what they should be and write 
> the values back to the flags, and subsequently asserts that the 
> internal and flag variants have the same value. Seems to me like we 
> could in a subsequent refactoring move some of that kind of stuff into 
> GCArguments instead, so that the argument setting logic is contained 
> in the same class rather than split all over the place. But that seems 
> like a separate RFE.
>
> Other than that, noticed some precompile header include was missing, 
> the GCArgumentProcessor::initialize() function returned jint but it 
> always returned JNI_OK, and called fatal() if something went wrong, 
> instead of logging an error and returning JNI_ERR. And now that these 
> types are collapsed, I moved initialize_flags_global into the abstract 
> initial_flags, allowing GCs to override it completely if necessary, 
> but in practice having them now call the super initial_flags(). So 
> yeah, went ahead and fixed that for you.
>
> As for your subsequent patch creating the heap, that will just be a 
> virtual call into the specific GCArguments class.
>
> I hope you agree with my proposal. It seemed easier for me to provide 
> a patch describing what I'm thinking than to write it down in text.
>
> Thanks,
> /Erik
>
> On 2017-10-23 17:19, Roman Kennke wrote:
>> Hi Erik,
>>
>> thanks for your suggestions. So I renamed 'GC' -> 
>> 'CollectedHeapFactory' (and subclasses likewise) and 'GCFactory' to 
>> 'GCArgumentProcessor'. This will make even more sense once I added 
>> heap creation to CollectedHeapFactory (JDK-8189389).
>>
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.02.diff/ 
>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02.diff/>
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.02/ 
>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.02/>
>>
>> Better?
>>
>> Roman
>>
>>> Hi Roman,
>>>
>>> I'm usually not one to really mind names too much, but I don't 
>>> believe some bootstrapping code for the GC should hog the name "GC".
>>>
>>> Instead of GC::initialize(), I'm thinking a static 
>>> GCArgumentProcessor::initialize() and instead of GC::factory(), I am 
>>> thinking a static CollectedHeapFactory::create(). Or something like 
>>> that.
>>>
>>> The reason is that I think the name GC is too general for the 
>>> bootstraping-only problems its code touches. Hope I am making sense.
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2017-10-20 14:25, Roman Kennke wrote:
>>>> Hi Erik,
>>>>
>>>> Yes that is fine.
>>>>
>>>> Keep in mind that I'm planning to put heap creation into that class 
>>>> too.
>>>>
>>>> I was thinking maybe to simply reverse the current naming: name the 
>>>> all-static 'factory factory' 'GC', and thus call GC::initialize() 
>>>> and GC::factory() or such, and the main interface GCFactory or 
>>>> CollectedHeapFactory. What do you think?
>>>>
>>>> Roman
>>>>
>>>>
>>>>> I would like to keep the CollectedHeap as the facade interface to 
>>>>> the GC, like it is today, instead of having a new GC class making 
>>>>> it two facade interfaces.
>>>>> Of course, the CollectedHeap may only be used as a facade after it 
>>>>> has been created. And now we are dealing with code before it was 
>>>>> created during the bootstrapping process.
>>>>>
>>>>> So what I would like to have here is a class specifically for 
>>>>> that, rather than another GC facade interface. I think this is 
>>>>> mostly an exercise of finding a better name for the class you call 
>>>>> "GC". Now I am not an expert in coming up with good names myself, 
>>>>> but some name that indicates this is being used for flag 
>>>>> processing would be good. Perhaps GCArgumentProcessor. The benefit 
>>>>> of calling it something like that is that if a GC requires 
>>>>> argument processing later in the bootstrapping, possibly after 
>>>>> CollectedHeap has been created, it can still be used for this 
>>>>> purpose without having to feel weird about it.
>>>>>
>>>>> How would you feel about that? Does it seem to make sense?
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>>>
>>>>> On 2017-10-19 21:14, Roman Kennke wrote:
>>>>>> Am 13.10.2017 um 03:20 schrieb David Holmes:
>>>>>>> Hi Roman,
>>>>>>>
>>>>>>> Not a review as GC folk need to do that.
>>>>>>>
>>>>>>> On 13/10/2017 5:59 AM, Roman Kennke wrote:
>>>>>>>> I'm posting it to both hotspot-runtime-dev and hotspot-gc-dev 
>>>>>>>> because it touches both areas (i.e. the GC interface).
>>>>>>>>
>>>>>>>> Currently, all GC related argument processing is done in 
>>>>>>>> arguments.cpp, littering it with #ifdef INCLUDE_ALL_GCS and all 
>>>>>>>> sorts of GC specific methods etc.
>>>>>>>
>>>>>>> From a runtime perspective I like all the GC specific ifdefs and 
>>>>>>> settings moved out-of-line of the main argument processing.
>>>>>>>
>>>>>>> From a refactoring perspective I noticed that 
>>>>>>> set_object_alignment() no longer calls set_cms_values(). I 
>>>>>>> presume setting it elsewhere is okay?
>>>>>> I totally forgot to reply to this.
>>>>>>
>>>>>> What's important is that the CMS alignment values are set after 
>>>>>> set_object_alignment() figured out the object alignment. The 
>>>>>> patch moves that a little further down the road to the beginning 
>>>>>> of its GC specific argument processing, but from GC perspective 
>>>>>> should be the same. I looked thoroughly through the involved code 
>>>>>> paths and cannot see what could go wrong.
>>>>>>
>>>>>> I need one more review. GC folks? The current webrev is:
>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.01/ 
>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.01/>
>>>>>>
>>>>>> Thanks,
>>>>>> Roman
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20171025/60efecba/attachment.htm>