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

[Coleen Phillimore]

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?

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