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

[David Holmes]

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