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

David Holmes david.holmes at oracle.com
Mon Nov 20 11:53:54 UTC 2017


On 20/11/2017 8:15 PM, Roman Kennke wrote:
> 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 couldn't understand how any minimal VM build could succeed**. I can 
only guess that it is due to precompiled headers pulling in 
arguments.hpp in one case but not another - though I can't say why 
fastdebug and slowdebug should behave differently ??

In JPRT we build product and fastdebug.

** I mistakenly thought the macro was declared in arguments.cpp and 
hence could never be available elsewhere.

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

Thanks,
David
-----
> 
> 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
>>>>>>
>>>>
>>>
> 


More information about the hotspot-gc-dev mailing list