RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Roger Riggs]

Hi Thomas,

No change was intended.

I'll file a bug and fix it shortly.

Roger


On 12/04/2018 08:07 AM, Thomas Stüfe wrote:
> Hi Roger,
>
> There is a difference now as to how java.specification.version is set.
>
> -    PUTPROP(props, "java.specification.version",
> -            VERSION_SPECIFICATION);
>
> +        props.setProperty("java.specification.version", VERSION_NUMBER);
>
> Was that intended?
>
> It seems to cause errors in our JTreg tests because the jtreg runner
> is expecting only the full version number without dots.
>
> Cheers, Thomas
>
>
>
> On Thu, Nov 15, 2018 at 2:12 AM Roger Riggs <Roger.Riggs at oracle.com> wrote:
>> 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
>>>>