RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Roger Riggs]

Hi Brent,


On 11/14/2018 06:54 PM, Brent Christian wrote:
> Hi, Roger
>
> * I like Mandy's idea of not combining the cli/VM props and the 
> well-known props into a single array.  Maybe we could then avoid 
> copying between arrays (System.c L166).
yes, it would avoid the copy.
>
> * The name "Raw" doesn't really speak to me.  It's OK as an inner 
> class, though I wonder if everything could be done in SystemProps 
> directly.
Raw to make it clear that these are not system properties, just data to 
be used to
create the system properties.

In some other prototyping, there were quite a few other changes in 
SystemProps
so keeping the primitive data separate was useful, possibly the class 
could be unloaded
since its use-once code.
>
> A few other comments:
>
> java.base/share/native/libjava/System.c:
>
>  112  * The first FIXED_LENGTH entries are the platform defined 
> property values, no names.
>  113  * The remaining array indices are alternating key/value pairs
>  114  * supplied by the VM including those defined on the command line
>  115  * using -Dkey=value that may override the platform defined value.
>
> Some of this would also be useful information SystemProps.java, maybe 
> in the comment for Raw.initProps.  Or refer the reader to 
> Java_jdk_internal_util_SystemProps_00024Raw_getRawProperties for a 
> description of the layout of the array.  Or may become moot if we use 
> two separate arrays.
Agreed, a more complete description would be good in the declaration of 
the native method.
>
> -- 
>
> file.encoding used to be set from sprops->encoding on Mac, and 
> sprops->sun_jnu_encoding otherwise:
>
>  382 #ifdef MACOSX
>  383         /*
>  384          * Since sun_jnu_encoding is now hard-coded to UTF-8 on 
> Mac, we don't
>  385          * want to use it to overwrite file.encoding
>  386          */
>  387         PUTPROP(props, "file.encoding", sprops->encoding);
>  388 #else
>  389         PUTPROP(props, "file.encoding", sprops->sun_jnu_encoding);
>  390 #endif
>
> There is no longer an ifdef for this.  But I guess this is OK, as
> SystemProps.initProperties() will only put a value for "file.encoding" 
> if there isn't one, and only uses sun.jnu.encoding if there is no 
> file.encoding:
>
>   59         putIfAbsent(props, "file.encoding",
>   60                 ((raw.propDefault(Raw._file_encoding_NDX) == null)
>   61                         ? raw.propDefault(Raw._sun_jnu_encoding_NDX)
>   62                         : raw.propDefault(Raw._file_encoding_NDX)));
>
Yes, I spent some time ensuring that the result was the same.
Thanks for confirming the logic.
> -- 
>
>  313     * Command line overrides with -D are copied above from 
> JVM_getProperties.
>
> s/JVM_get/JVM_Get/
right, fixed, Java naming conventions bleeding into native.
>
>
> java.base/share/classes/jdk/internal/util/SystemProps.java:
>
>  126     /**
>  127      * Puts the property if it is non-null
>  128      * @param props the Properties
>  129      * @param key the key
>  130      * @param value the value
>  131      */
>  132     private static void put(Properties props, String key, String 
> value) {
>
> I notice that this method is only called once.
Yes, at the moment, that is the only property that must not be 
overridden on the command line.
There are others that probably should not be overridable but for 
compatibility...
>
> -- 
>
>  175             cmdProps.put(disp, dispValue);
> ...
>  186             cmdProps.put(fmt, fmtValue);
> ) {
>
>         } else if (
> Aren't L175 & L186 just putting the property that is already there ?
Oops,  that's a carryover from a different implementation that had 
separate maps.
Those can be removed since an I18N property on the command is always 
retained
only defined if the platform value is supplied and different than the base.

Thanks for the review and suggestions

Roger

>
> Thanks,
> -Brent
>
> On 11/13/18 7:59 AM, Roger Riggs wrote:
>> Please review a re-implementation of the initialization of System 
>> properties
>> moving some functions from native to Java.
>>
>> The upcalls from native to java for each property are replaced by a 
>> mechanism
>> to gather the platform, VM and command line properties and return them
>> from a single JNI call.  The creation of the Properties instance and 
>> applying
>> command line overrides is handled in Java instead of native.
>>
>> The JVM_initProperties interface in Hotspot is replaced by 
>> JVM_getProperties.
>>
>> Webrev:
>>    http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/
>>
>> Issue:
>>    https://bugs.openjdk.java.net/browse/JDK-4947890
>>
>> Thanks, Roger
>>