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

[Erik Österlund]

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