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

[Roman Kennke]

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