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.
<snip>
src/share/vm/runtime/globals_ext.hpp
With all the
inline bool Flag::is_valid_ext_T(T value, FlagValueOrigin origin)
functions, is it necessary to include the type T in the function name?
It is necessary if using type safe variants with T value as argument
since overloading does not differ between different typedef names that
resolves to the same native types, e.g. uintx and uint64_t are both
unsigned long int.
I am considering a condensed variant that replaces T by void* instead,
and do the type casting based on the targeted flag, reducing the number
of functions.
Ok. I thought there might be some conflict with the integral types.
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.
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.
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
