RFR 4947890 : Minimize JNI upcalls in system property initialization

Tue Nov 20 11:10:49 UTC 2018

On 2018-11-20 00:37, Roger Riggs wrote:

> Hi,
>
> Webrev updated in place:
>
>     http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw
Build changes still look fine. But please, in the future, avoid updating 
webrevs in place, but create new ones instead. It's hopeless to figure 
out what has changed otherwise. :-( Updating in place is OK for things 
like typos, but not actual code changes...

/Magnus

>
> On 11/16/2018 06:36 PM, Mandy Chung wrote:
>> Hi Roger,
>>
>> Looking good.  I have a few small comments:
>>
>> I assume VM.saveAndRemoveProperties will be a separate cleanup.
>> SystemProps::cmdProperties adds the system properties that can
>> skip adding these internal properties to the Properties object.
>> In fact, these internal properties are the interface between VM
>> and libraries that can be replaced with a different mechanism
>> other than system properties.
> fine as a future change
>>
>> Suggest to move the call to VersionProps.init(props) in
>> SystemProps.initProperties
> That's harder that it might seems, the method can't be made public
> and moving it to jdk.internal.util with SystemProps is more difficult
> since there is Hotspot code that reads those fields directly. 
> (thread.cpp)
> That goes back quiet a number of years.
>
> Another future cleanup/decoupling.
>>
>> The `VENDOR*` build time variables can always have a default
>> like the other constants in VersionProps.  Then no need to
>> handle if not set.
> Moved defaults to configure,
> easy to override with --with-vendor-url, --with-vendor-bug-url, and 
> --with-vendor-name.
>>
>> SystemProps.staticInitOnly_* are a bit odd.  They are temporarily
>> set with the values and then reset to null.  Perhaps leave
>> StaticProperty as is for now and re-visit it in the future.
>> Then I think SystemProps::putDefault can be removed.
> ok, reverted the code so the static values are read via 
> System.getProperty.
>>
>> Raw::xxx_NDX are initialized to 1 + previous_NDX.  It's a general
>> good approach to increment the index but I find it error-prone and
>> hard to catch mistake since the (adjacent) variable names look
>> so alike. Perhaps some form of verification or assertion to ensure
>> the indices are correctly initialized.
> Added a test that uses reflection to verify the uniqueness and sequence.
>
> Thanks, Roger
>>
>> Mandy
>>
>> On 11/16/18 10:49 AM, Roger Riggs wrote:
>>> Updates to include the suggestions made by Mandy and Brent:
>>>
>>>  - Move the build-time properties from native code to the 
>>> VersionProps.java.template
>>>    including java.vendor.* and java.specification.* properties, plus 
>>> the java.class.version (major.minor)
>>>    This included two changes to the build that generates source of 
>>> VersionProps.java.
>>>
>>>  - Updated the StaticProperty initialization to be explicitly 
>>> invoked by initProperties.
>>>
>>>  - Makes separate calls to native to retrieve the platform 
>>> properties and VM/command line properties
>>>
>>>  - (The hotspot function for JVM_GetProperties are unchanged)
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/
>>>
>>> Issue:
>>> https://bugs.openjdk.java.net/browse/JDK-4947890
>>>
>>> Thanks, Roger
>>>
>>>
>>
>