RFR 4947890 : Minimize JNI upcalls in system-properties initialization

Tue Dec 4 14:18:18 UTC 2018

Thanks Roger!
On Tue, Dec 4, 2018 at 3:14 PM Roger Riggs <Roger.Riggs at oracle.com> wrote:
>
> 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
>
>