RFR (S): 8204668: Cleanup management of the java.vm.info System property

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (S): 8204668: Cleanup management of the java.vm.info System property

David Holmes
Bug: https://bugs.openjdk.java.net/browse/JDK-8204668
webrev: http://cr.openjdk.java.net/~dholmes/8204668/webrev/

JDK-8203329 fixed a problem where the native system property for the
vm.info string was not updated after argument parsing, resulting in JVM
TI reporting an incorrect value.

Looking at the overall approach for this property it can be simplified
quite a bit. The basic issue is that it is initialized early in VM
startup (so it can be present for crash logs) before argument parsing,
but some details can change due to argument parsing. If we update the
native value immediately after argument parsing, and so before the
properties are passed through to the Java side, then we don't need to
execute the Java code in reset_vm_info() to perform that update.
Additionally, if we expose the SystemProperty directly (as done for
other properties) then we can do away with the new
PropertyList_update_value() function that has to search for the property
to be updated.

Overall this cuts out a chunk of initialization code that may aid with
startup costs; and simplifies the code.

There's some additional history in the bug report.

Testing:
   - tier 1, 2, 3
   - regression test from JDK-8203329:
      -
serviceability/jvmti/GetSystemProperty/JvmtiGetSystemPropertyTest.java

Thanks,
David
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8204668: Cleanup management of the java.vm.info System property

Robbin Ehn
Hi David, looks good, thanks for fixing!

/Robbin

On 06/12/2018 11:56 AM, David Holmes wrote:

> Bug: https://bugs.openjdk.java.net/browse/JDK-8204668
> webrev: http://cr.openjdk.java.net/~dholmes/8204668/webrev/
>
> JDK-8203329 fixed a problem where the native system property for the vm.info
> string was not updated after argument parsing, resulting in JVM TI reporting an
> incorrect value.
>
> Looking at the overall approach for this property it can be simplified quite a
> bit. The basic issue is that it is initialized early in VM startup (so it can be
> present for crash logs) before argument parsing, but some details can change due
> to argument parsing. If we update the native value immediately after argument
> parsing, and so before the properties are passed through to the Java side, then
> we don't need to execute the Java code in reset_vm_info() to perform that
> update. Additionally, if we expose the SystemProperty directly (as done for
> other properties) then we can do away with the new PropertyList_update_value()
> function that has to search for the property to be updated.
>
> Overall this cuts out a chunk of initialization code that may aid with startup
> costs; and simplifies the code.
>
> There's some additional history in the bug report.
>
> Testing:
>    - tier 1, 2, 3
>    - regression test from JDK-8203329:
>       - serviceability/jvmti/GetSystemProperty/JvmtiGetSystemPropertyTest.java
>
> Thanks,
> David
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8204668: Cleanup management of the java.vm.info System property

David Holmes
Thanks Robbin!

David

On 12/06/2018 9:51 PM, Robbin Ehn wrote:

> Hi David, looks good, thanks for fixing!
>
> /Robbin
>
> On 06/12/2018 11:56 AM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204668
>> webrev: http://cr.openjdk.java.net/~dholmes/8204668/webrev/
>>
>> JDK-8203329 fixed a problem where the native system property for the
>> vm.info string was not updated after argument parsing, resulting in
>> JVM TI reporting an incorrect value.
>>
>> Looking at the overall approach for this property it can be simplified
>> quite a bit. The basic issue is that it is initialized early in VM
>> startup (so it can be present for crash logs) before argument parsing,
>> but some details can change due to argument parsing. If we update the
>> native value immediately after argument parsing, and so before the
>> properties are passed through to the Java side, then we don't need to
>> execute the Java code in reset_vm_info() to perform that update.
>> Additionally, if we expose the SystemProperty directly (as done for
>> other properties) then we can do away with the new
>> PropertyList_update_value() function that has to search for the
>> property to be updated.
>>
>> Overall this cuts out a chunk of initialization code that may aid with
>> startup costs; and simplifies the code.
>>
>> There's some additional history in the bug report.
>>
>> Testing:
>>    - tier 1, 2, 3
>>    - regression test from JDK-8203329:
>>       -
>> serviceability/jvmti/GetSystemProperty/JvmtiGetSystemPropertyTest.java
>>
>> Thanks,
>> David
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8204668: Cleanup management of the java.vm.info System property

Chris Plummer
In reply to this post by David Holmes
Hi David,

Is there a reason you moved the PropertyList_add() call to a slightly
later point?

You moved the Arguments::update_vm_info_property() call up a bit. How
did you decide exactly how early it can be called without risk of other
flags changing?

thanks,

Chris

On 6/12/18 2:56 AM, David Holmes wrote:

> Bug: https://bugs.openjdk.java.net/browse/JDK-8204668
> webrev: http://cr.openjdk.java.net/~dholmes/8204668/webrev/
>
> JDK-8203329 fixed a problem where the native system property for the
> vm.info string was not updated after argument parsing, resulting in
> JVM TI reporting an incorrect value.
>
> Looking at the overall approach for this property it can be simplified
> quite a bit. The basic issue is that it is initialized early in VM
> startup (so it can be present for crash logs) before argument parsing,
> but some details can change due to argument parsing. If we update the
> native value immediately after argument parsing, and so before the
> properties are passed through to the Java side, then we don't need to
> execute the Java code in reset_vm_info() to perform that update.
> Additionally, if we expose the SystemProperty directly (as done for
> other properties) then we can do away with the new
> PropertyList_update_value() function that has to search for the
> property to be updated.
>
> Overall this cuts out a chunk of initialization code that may aid with
> startup costs; and simplifies the code.
>
> There's some additional history in the bug report.
>
> Testing:
>   - tier 1, 2, 3
>   - regression test from JDK-8203329:
>      -
> serviceability/jvmti/GetSystemProperty/JvmtiGetSystemPropertyTest.java
>
> Thanks,
> David



Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8204668: Cleanup management of the java.vm.info System property

David Holmes
Hi Chris,

Thanks for looking at this.

On 13/06/2018 3:52 AM, Chris Plummer wrote:
> Hi David,
>
> Is there a reason you moved the PropertyList_add() call to a slightly
> later point?

Just following the existing pattern in that method: define the
SystemProperty objects, then later add them.

> You moved the Arguments::update_vm_info_property() call up a bit. How
> did you decide exactly how early it can be called without risk of other
> flags changing?

Trial and error. I originally thought it was safe to do this once all
the argument processing was complete: ergo changes, OS changes etc. But
in fact the two flags we are concerned with do not get their final value
until somewhere deep in init_globals(). If I put the update before
init_globals() then it had the wrong value; if I put it after then it
was correct. The main thing was to move it before the initialization of
the system classes which would push the value through to the Java side.

Thanks,
David

> thanks,
>
> Chris
>
> On 6/12/18 2:56 AM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204668
>> webrev: http://cr.openjdk.java.net/~dholmes/8204668/webrev/
>>
>> JDK-8203329 fixed a problem where the native system property for the
>> vm.info string was not updated after argument parsing, resulting in
>> JVM TI reporting an incorrect value.
>>
>> Looking at the overall approach for this property it can be simplified
>> quite a bit. The basic issue is that it is initialized early in VM
>> startup (so it can be present for crash logs) before argument parsing,
>> but some details can change due to argument parsing. If we update the
>> native value immediately after argument parsing, and so before the
>> properties are passed through to the Java side, then we don't need to
>> execute the Java code in reset_vm_info() to perform that update.
>> Additionally, if we expose the SystemProperty directly (as done for
>> other properties) then we can do away with the new
>> PropertyList_update_value() function that has to search for the
>> property to be updated.
>>
>> Overall this cuts out a chunk of initialization code that may aid with
>> startup costs; and simplifies the code.
>>
>> There's some additional history in the bug report.
>>
>> Testing:
>>   - tier 1, 2, 3
>>   - regression test from JDK-8203329:
>>      -
>> serviceability/jvmti/GetSystemProperty/JvmtiGetSystemPropertyTest.java
>>
>> Thanks,
>> David
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8204668: Cleanup management of the java.vm.info System property

Chris Plummer
Ok. Changes look good to me.

thanks,

Chris

On 6/12/18 2:37 PM, David Holmes wrote:

> Hi Chris,
>
> Thanks for looking at this.
>
> On 13/06/2018 3:52 AM, Chris Plummer wrote:
>> Hi David,
>>
>> Is there a reason you moved the PropertyList_add() call to a slightly
>> later point?
>
> Just following the existing pattern in that method: define the
> SystemProperty objects, then later add them.
>
>> You moved the Arguments::update_vm_info_property() call up a bit. How
>> did you decide exactly how early it can be called without risk of
>> other flags changing?
>
> Trial and error. I originally thought it was safe to do this once all
> the argument processing was complete: ergo changes, OS changes etc.
> But in fact the two flags we are concerned with do not get their final
> value until somewhere deep in init_globals(). If I put the update
> before init_globals() then it had the wrong value; if I put it after
> then it was correct. The main thing was to move it before the
> initialization of the system classes which would push the value
> through to the Java side.
>
> Thanks,
> David
>
>> thanks,
>>
>> Chris
>>
>> On 6/12/18 2:56 AM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204668
>>> webrev: http://cr.openjdk.java.net/~dholmes/8204668/webrev/
>>>
>>> JDK-8203329 fixed a problem where the native system property for the
>>> vm.info string was not updated after argument parsing, resulting in
>>> JVM TI reporting an incorrect value.
>>>
>>> Looking at the overall approach for this property it can be
>>> simplified quite a bit. The basic issue is that it is initialized
>>> early in VM startup (so it can be present for crash logs) before
>>> argument parsing, but some details can change due to argument
>>> parsing. If we update the native value immediately after argument
>>> parsing, and so before the properties are passed through to the Java
>>> side, then we don't need to execute the Java code in reset_vm_info()
>>> to perform that update. Additionally, if we expose the
>>> SystemProperty directly (as done for other properties) then we can
>>> do away with the new PropertyList_update_value() function that has
>>> to search for the property to be updated.
>>>
>>> Overall this cuts out a chunk of initialization code that may aid
>>> with startup costs; and simplifies the code.
>>>
>>> There's some additional history in the bug report.
>>>
>>> Testing:
>>>   - tier 1, 2, 3
>>>   - regression test from JDK-8203329:
>>>      -
>>> serviceability/jvmti/GetSystemProperty/JvmtiGetSystemPropertyTest.java
>>>
>>> Thanks,
>>> David
>>
>>
>>


Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8204668: Cleanup management of the java.vm.info System property

David Holmes
Thanks Chris.

I added some more info on the initialization sequence to the bug report.
And filed a related bug on the LogVMOutput output.

David

On 13/06/2018 8:29 AM, Chris Plummer wrote:

> Ok. Changes look good to me.
>
> thanks,
>
> Chris
>
> On 6/12/18 2:37 PM, David Holmes wrote:
>> Hi Chris,
>>
>> Thanks for looking at this.
>>
>> On 13/06/2018 3:52 AM, Chris Plummer wrote:
>>> Hi David,
>>>
>>> Is there a reason you moved the PropertyList_add() call to a slightly
>>> later point?
>>
>> Just following the existing pattern in that method: define the
>> SystemProperty objects, then later add them.
>>
>>> You moved the Arguments::update_vm_info_property() call up a bit. How
>>> did you decide exactly how early it can be called without risk of
>>> other flags changing?
>>
>> Trial and error. I originally thought it was safe to do this once all
>> the argument processing was complete: ergo changes, OS changes etc.
>> But in fact the two flags we are concerned with do not get their final
>> value until somewhere deep in init_globals(). If I put the update
>> before init_globals() then it had the wrong value; if I put it after
>> then it was correct. The main thing was to move it before the
>> initialization of the system classes which would push the value
>> through to the Java side.
>>
>> Thanks,
>> David
>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 6/12/18 2:56 AM, David Holmes wrote:
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204668
>>>> webrev: http://cr.openjdk.java.net/~dholmes/8204668/webrev/
>>>>
>>>> JDK-8203329 fixed a problem where the native system property for the
>>>> vm.info string was not updated after argument parsing, resulting in
>>>> JVM TI reporting an incorrect value.
>>>>
>>>> Looking at the overall approach for this property it can be
>>>> simplified quite a bit. The basic issue is that it is initialized
>>>> early in VM startup (so it can be present for crash logs) before
>>>> argument parsing, but some details can change due to argument
>>>> parsing. If we update the native value immediately after argument
>>>> parsing, and so before the properties are passed through to the Java
>>>> side, then we don't need to execute the Java code in reset_vm_info()
>>>> to perform that update. Additionally, if we expose the
>>>> SystemProperty directly (as done for other properties) then we can
>>>> do away with the new PropertyList_update_value() function that has
>>>> to search for the property to be updated.
>>>>
>>>> Overall this cuts out a chunk of initialization code that may aid
>>>> with startup costs; and simplifies the code.
>>>>
>>>> There's some additional history in the bug report.
>>>>
>>>> Testing:
>>>>   - tier 1, 2, 3
>>>>   - regression test from JDK-8203329:
>>>>      -
>>>> serviceability/jvmti/GetSystemProperty/JvmtiGetSystemPropertyTest.java
>>>>
>>>> Thanks,
>>>> David
>>>
>>>
>>>
>
>