Review Request JDK-8136930 Examine implications for custom launchers, equivalent of java -X options in particular

[David Holmes]

On 9/06/2016 8:37 AM, Coleen Phillimore wrote:
>
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.01/hotspot/src/share/vm/runtime/arguments.cpp.udiff.html
>
>
> It took me a while to see why build_jvm_args() is called for the option
> strings for -addmods, -limitmods, etc. because of the whitespace.  Can
> there be a short comment somewhere?  Maybe at the top of build_jvm_args
> function definition, something like "options skipped by the main
> parse_each_vm_init_arg loop must be added explicitly" or similar.  I
> don't think each of the 4 calls should have the comment, it would be
> redundant.
>
> I have to ask why Hotspot convention was violated with this new option
> syntax?  These options don't start with -X and the values aren't
> specified as : separators like the rest? Like
> -Xshare:{dump,auto,on,off}, etc.  Why are these different?

Good question. Presumably these are not non-standard arguments, but 
common arguments that must always be supported, hence not -X. But this 
somewhat muddies the waters as the VM itself has, to my recollection, 
never had standard arguments: VM arguments are -XX arguments, other 
arguments were launcher arguments. So yes this seems to be introducing a 
new kind of VM argument.

This also begs the questions as to where you get help for these options 
once moved into the JVM? Will the launcher be updated to pretend they 
are launcher arguments and document them?

I agree with Coleen's suggested improvements to the code.

David
-----


> + } else if (match_option(option, "-Xpatch:", &tail)) {
>
>
> Can the parsing of -Xpatch be an out of line function?  This series of
> '} else if (match_option(option) ..." code is already too long.
>
> + // Append the value of the last -addmods option specified on the
> command line.
>
>
> Is there a test for this?  So -addmods uses the last one specified but
> the others -limitmods, -upgrademodulepath -modulepath, etc, add
> properties as found on the command line?
>
> Someone already asked this but are there tests for all these
> combinations of options?
>
>
> + PropertyList_unique_add(&_system_properties, key, value, true, true,
> false);
>
>
> Can there be 3 enums:
> enum { WriteableProperty, UnwriteableProperty };
> enum { InternalProperty, ExternalProperty };
> enum { AppendProperty, AddProperty };
>
> And pass enum values rather than true, false, true, etc.  These serve
> the purpose of documenting the arguments and preventing you or your
> successors from mixing up which boolean is which.
>
> -void Arguments::PropertyList_unique_add(SystemProperty** plist, const
> char* k, const char* v, jboolean append) {
> +void Arguments::PropertyList_unique_add(SystemProperty** plist, const
> char* k, const char* v,
> + jboolean append, bool writeable, bool internal) {
>
>
> You should change the jboolean argument in any case.
>
>
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.01/hotspot/src/share/vm/runtime/arguments.hpp.udiff.html
>
>
>
> + // Return TRUE if option matches property or matches property=.
> + static bool is_matching_property(const char* option, const char*
> property, size_t len) {
> + return (strncmp(option, property, len) == 0) && (option[len] == '=' ||
> option[len] == '\0');
> + }
> +
> + // Return TRUE if option matches property.<digits> or matches
> property.<digits>=.
> + static bool is_matching_numbered_property(const char* option, const
> char* property, size_t len);
>
>
> I think these functions can be static internal functions and not
> contained to the Arguments class, since they don't use instance data of
> Arguments and aren't exported.
>
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.01/hotspot/test/runtime/modules/IgnoreModulePropertiesTest.java.html
>
>
> This test should include cases for specifying multiple of these
> arguments on the command line, as well as not having values to the
> whitespace arguments,either on the end of the command line or in the
> middle (what happens if you have -addmods -XX:-UseCompressedOops, for
> example)?
>
>
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.01/jdk/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java.udiff.html
>
>
> + /**
> + * Gets and remove the named system property
> + */
> + private static String getAndRemoveProperty(String key) {
> + return (String)System.getProperties().remove(key);
> + }
>
> There are two of these.  Should there be only one in the System class?
>
> The VM changes seem to be the most significant part of this change so
> bypassing jvm nightly testing in hs repository seems like a mistake to
> me, but the people who watch for test failures and gatekeeping are okay
> with pushing this to jdk9/dev.  So I won't complain.  Harold's assured
> me that he's run all the equivalent of nightly tests on this.
>
> Thanks,
> Coleen
>
>
> On 6/8/16 12:35 PM, Mandy Chung wrote:
>> Updated webrev with Harold’s latest VM change incorporating the review
>> comments:
>>
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.01/index.html
>>
>>
>> Harold - the revised VM change looks okay to me.
>>
>> Minor one: you define the following:
>>   202 #define MODULE_PATH_PROPERTY "-Djdk.module.path”
>>   204 #define MODULE_UPGRADE_PATH_PROPERTY "-Djdk.module.upgrade.path"
>>
>> It may be good to consider having #define for all module property
>> names and used consistently.
>>
>> Mandy
>>
>>> On Jun 3, 2016, at 11:47 PM, Mandy Chung <mandy.chung at oracle.com> wrote:
>>>
>>> Webrev:
>>>   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.00/
>>>
>>> -modulepath, -addmods, -limitmods, -XaddExports, -XaddReads, -Xpatch
>>> are java launcher options in the current implementation.  Custom
>>> launchers will have to use -D to set some system properties to
>>> configure module system.  Different ways to configure module system
>>> is confusing and not friendly for environments using both java
>>> launcher and custom launchers.
>>>
>>> This patch pushes the handling of the module options into the VM.
>>> That will avoid the confusion between launcher and VM options and
>>> avoids needing to use system properties.  All launcher
>>> implementations can configure the module system via JNI Invocation
>>> API setting these options in a unified way.  The options and syntax
>>> remain the same as specified in JEP 261.
>>>
>>> For the non-repeating options, like the other VM options, the last
>>> one wins.  The current implementation communicates the options to the
>>> module system through system properties, as a private interface, and
>>> these system properties will be removed once they are read during the
>>> module system initialization.  These system properties are reserved
>>> as private interface and they will be ignored if they are set via -D
>>> in the command line. Harold implements the hotspot change and can
>>> explain further details.
>>>
>>> This patch will impact existing tests and scripts that set the system
>>> properties for example to break encapsulation in the command line
>>> e.g. -Djdk.launcher.addexports.<N>.  They will need to be updated to
>>> replace the use of -D with the appropriate module option e.g.
>>> -XaddExports.  Since they are new options in JDK 9, use
>>> -XX:+IgnoreUnrecognizedVMOptions if they need to be ignored by
>>> earlier releases.
>>>
>>> Mandy
>>>
>>>
>