RFR 4947890 : Minimize JNI upcalls in system-properties initialization
Thomas Stüfe
thomas.stuefe at gmail.com
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
>
>
More information about the core-libs-dev
mailing list