RFR (S): 7130391: Add a framework for vendor-specific validation control of setting command-line switches in Hotspot

[Robert Ottenhag]

Hi David,

More comments inline...

On 01/18/2012 04:36 AM, David Holmes wrote:
> Hi Robert,
>
> Comments inline ...
>
> On 17/01/2012 10:07 PM, Robert Ottenhag wrote:
>>> On 17/01/2012 12:04 PM, Robert Ottenhag wrote:
>>>> Webrev: http://cr.openjdk.java.net/~rottenha/7130391/webrev.00
>>>>
>>>> This fix adds optional validation control to the setting of
>>>> command-line switches in Hotspot, and allows it to have
>>>> vendor-specific extensions if necessary.
>>>
>>> Does this imply that the Java management APIs (eg
>>> com.sun.management.VMOption) need to be changed to reflect these
>>> restrictions? Presently VMOptions are either writeable or not, but
>>> this makes them conditionally-writeable.
>>
>> No, since the Java management APIs already cares for conditional writes.
>> According to com.sun.management.HotSpotDiagnosticMXBean.setVMOption() it
>> will throw IllegalArgumentException if the new value is invalid.
>
> I think there is a significant difference between trying to set an 
> invalid value and setting to a valid value at a time that it is not 
> permitted.

Agreed. There is a semantic difference here that can be confusing.

<snip>
>
>>> src/share/vm/runtime/globals.cpp
>>>
>>> The use of the guarantees seems wrong as it means an invalid option
>>> will trigger a VM crash rather than a clean exit during
>>> initialization. It seems to me that none of the code in arguments.cpp
>>> that uses the FLAG_SET_* macros (which in turn use the
>>> CommandLineFlagsEx functions you added the guarantees to) anticipates
>>> any possibility for failure. I think if you are going this path then
>>> you have no choice but to change the CommandLineFlagsEx methods to
>>> return bool and update the FLAG_SET macros to try and perform
>>> appropriate error handling.
>>
>> I see your point, and in theory such as VM crash could occur anytime
>> later in a JVM session if rarely running code would make use of
>> FLAG_SET_* to change the value of a VM flag to an invalid value or 
>> origin.
>>
>> Seems as if the options are either to
>> a) ignore validation tests for the FLAG_SET_* macros, and trust that
>> they always set valid values. This can be partly verified by static code
>> inspection by looking for any variables that actually have validation
>> logic associated to them (since the variable name is defined at compile
>> time), assuming one has access to all code, but it is not perfect in
>> case code for changing a variable with validation logic exists.
>> b) contain the error handling within the FLAG_SET_* macros, like using
>> guarantee(), but maybe exception logic can help?
>> c) require usage of the FLAG_SET_* macros to handle result codes and
>> pass it up the call chain.
>>
>> Also, the current macro FLAG_SET_DEFAULT does a direct write to the flag
>> value without going through <type>AtPut(). This macro must be rewritten
>> to have validation control to close the holes. The current call format
>> will require all call sites to include type name as with
>> FLAG_SET_{CMDLINE,ERGO} has, or to use slower lookup by variable name.
>
> I think you touched on the real problem in your later email - really 
> these flags/options and the ways you can interact with them should be 
> encapsulated in objects. Each different flag can then define its valid 
> values, whether it is "locked", "writeable" etc. But that means every 
> use of those flags in the VM would need changing - which is indeed a 
> very intrusive change.

We might be able to keep existing usage by encapsulating it within a 
class that overrides the assignment and type conversion operators, but 
as you say it is a little more work than expected right now.

>
> But I can't help but feel that we are going to far in what we are 
> trying to do with these flags when they are in fact simple variables.
>
> Also I think we may be overcomplicating this. I don't see why we can't 
> consider the uses of the flags at initialization time and runtime to 
> be distinct use-cases and use different APIs to interact with them. 
> For initialization we have the FLAGS_SET_* macros, and the end result 
> is that we have a set of flags that are either at their default values 
> or have been set to a valid value. I don't think we need to consider 
> (as I believe the current proposal does) multiple settings of a given 
> flag at initialization time ie:
>
> java -XX:+UseFoo ... -XX:-UseFoo ... -XX:+UseFoo
>
> should simply result in UseFoo==true. Even if we have stated that once 
> UseFoo is turned on it can't be turned off again. To me that should 
> only relate to true "dynamic" runtime setting of the flags. In which 
> case only the management APIs need to be augmented to support this and 
> we may be able to create "shadow" objects for flags we need to handle 
> specially at runtime.

The problem with the FLAG_SET_* macros is that they can also be used 
after initialization, or for that matter direct variable assignment can 
be also done, to bypass any validation logic that is observed by the 
dynamic interfaces.

However, at this point I am also leaning towards a design that only 
focuses on the dynamic setting, which will not change existing behavior 
of command line flags parsing, i.e. any variable is writable with any 
value during the initialization phase.

>
> David
> -----
>
>> /Robert
>>
>>>
>>> David
>>> -----
>>>
>>>
>>>> A simple use case for validation is a manageable flag whose current
>>>> value can not be less than the previous value, while a more complex
>>>> example may base the validation on multiple other flags, etc.
>>>>
>>>> Thanks,
>>>>
>>>> /Robert
>>>>
>>
>>


-- 
Oracle
Robert Ottenhag | Senior Member of Technical Staff
Phone: +46850630961 | Fax: +46850630911 | Mobile: +46707106161
Oracle Java HotSpot Virtual Machine
ORACLE Sweden | Folkungagatan 122 | SE-116 30 Stockholm

Oracle Svenska AB, Kronborgsgränd 17, S-164 28 KISTA, reg.no. 556254-6746

Green Oracle

Oracle is committed to developing practices and products that help protect the environment
--