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-runtime-dev
mailing list