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
