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

[Roman Kennke]

Hi David,

I just built linux-x86_64-normal-minimal-slowdebug and it does not fail. 
That's what I did when I tested the patch(es). It's not reasonable to 
expect devs to try and build all possible combinations of jvm variants, 
debug-levels, or even arches etc.

I'll file and fix a follow-up bug for it.

Roman

> On 17/11/2017 2:33 AM, Bob Vandette wrote:
>> Yes, that’s the fix.  I built it correctly with that added include.
>
> How? I can't build the MinimalVM in JPRT because the 
> UNSUPPORTED_OPTION macro is only locally defined in arguments.cpp, but 
> is now used in gcArguments.cpp:
>
> /opt/jprt/T/P1/003543.daholme/s/open/src/hotspot/share/gc/shared/gcArguments.cpp:77:29: 
> error: 'UNSUPPORTED_OPTION' was not declared in this scope
>    UNSUPPORTED_OPTION(UseG1GC);
>                              ^
> make[3]: *** 
> [/opt/jprt/T/P1/003543.daholme/s/build/linux-x86-debug/hotspot/variant-minimal/libjvm/objs/gcArguments.o] 
> Error 1
>
> ---
>
> Roman/Erik: If changing code that has #include guards you must ensure 
> all the paths are tested!
>
> Thanks,
> David
>
>> There are still a few other issues with building and using the 
>> minimal VM but those
>> existed before your change.
>>
>> 1. You cannot build the minimal VM without also building the server 
>> VM.  There
>> is a Makefile dependency on the server VM.
>> 2. The jvm.cfg produced in the jvm and jre images does not add the 
>> minimalvm
>> as one of the possible VM to select.  As a work-around jlink produces 
>> a workable jvm.cfg.
>>
>> Thanks,
>> Bob.
>>
>>
>>> On Nov 16, 2017, at 11:17 AM, Roman Kennke <rkennke at redhat.com> wrote:
>>>
>>> Hi Bob,
>>> thanks for letting me know.
>>>
>>> I filed: https://bugs.openjdk.java.net/browse/JDK-8191424
>>> and posted for review: 
>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2017-November/020784.html
>>>
>>> Thanks, Roman
>>>
>>>> Roman,
>>>>
>>>> It looks like this change may have caused the build of the minimal 
>>>> VM to
>>>> fail.  The minimal VM is no longer a configuration that we 
>>>> regularly build and test
>>>> but it is still a build option that can be selected via 
>>>> "--with-jvm-variants=minimal1"
>>>>
>>>>
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp: 
>>>> In static member function 'static jint GCArguments::initialize()':
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:113:17: 
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseParallelGC not 
>>>> supported in this VM.\n");
>>>>                   ^
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:116:17: 
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), "UseG1GC not 
>>>> supported in this VM.\n");
>>>>                   ^
>>>> /scratch/users/bobv/jdk10hs/open/src/hotspot/share/gc/shared/gcArguments.cpp:119:17: 
>>>> error: 'defaultStream' has not been declared
>>>>       jio_fprintf(defaultStream::error_stream(), 
>>>> "UseConcMarkSweepGC not supported in this VM.\n");
>>>>                   ^
>>>> gmake[3]: *** 
>>>> [/scratch/users/bobv/jdk10hs/build/b00/linux-x64-minimal1/hotspot/variant-minimal/libjvm/objs/gcArguments.o] 
>>>> Error 1
>>>>
>>>> Bob.
>>>>
>>>>
>>>>> On Nov 7, 2017, at 6:01 AM, Roman Kennke <rkennke at redhat.com> wrote:
>>>>>
>>>>> Hi Per & Erik,
>>>>>
>>>>> thanks for the reviews!
>>>>>
>>>>> Now I need a sponsor.
>>>>>
>>>>> Final webrev (same as before, including Reviewed-by line):
>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.05/ 
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.05/>
>>>>>
>>>>> Thanks, Roman
>>>>>
>>>>>
>>>>>> Looks good Roman!
>>>>>>
>>>>>> /Per
>>>>>>
>>>>>> On 2017-11-06 12:13, Roman Kennke wrote:
>>>>>>> Am 26.10.2017 um 11:43 schrieb Per Liden:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 2017-10-25 18:11, Erik Osterlund wrote:
>>>>>>>> [...]
>>>>>>>>>> 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/>
>>>>>>>> I like this. Just two naming suggestions:
>>>>>>>>
>>>>>>>> src/hotspot/share/gc/shared/gcArguments.hpp:
>>>>>>>>
>>>>>>>>    39   static jint create_instance();
>>>>>>>>    40   static bool is_initialized();
>>>>>>>>    41   static GCArguments* instance();
>>>>>>>>
>>>>>>>> Could we call these:
>>>>>>>>
>>>>>>>>    39   static jint initialize();
>>>>>>>>    40   static bool is_initialized();
>>>>>>>>    41   static GCArguments* arguments();
>>>>>>>>
>>>>>>>> Reasoning: initialize() maps better to its companion 
>>>>>>>> is_initialized().
>>>>>>>> GCArguments::arguments() maps better to the existing patterns 
>>>>>>>> we have
>>>>>>>> with CollectedHeap::heap().
>>>>>>> Ok, that sounds good to me. Actually, it's almost full-circle 
>>>>>>> back to my
>>>>>>> original proposal ;-)
>>>>>>>
>>>>>>> Differential:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04.diff/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04.diff/>
>>>>>>> Full:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8189171/webrev.04/
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8189171/webrev.04/>
>>>>>>>
>>>>>>> Ok to push now?
>>>>>>>
>>>>>>> Roman
>>>>>
>>>
>>