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.ri...@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
> >>
>
