RFR 4947890 : Minimize JNI upcalls in system-properties initialization
Roger Riggs
Roger.Riggs at oracle.com
Tue Dec 4 14:14:11 UTC 2018
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
>>>>
More information about the core-libs-dev
mailing list