RFR (S): 8146820: JVMCI properties should use HotSpotJVMCIRuntime.getBooleanProperty mechanism
Christian Thalinger
christian.thalinger at oracle.com
Thu Jan 14 21:48:32 UTC 2016
> On Jan 14, 2016, at 2:44 AM, Doug Simon <doug.simon at oracle.com> wrote:
>
>>
>> On 14 Jan 2016, at 06:58, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>
>>>
>>> On Jan 12, 2016, at 12:39 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>
>>>>
>>>> On Jan 12, 2016, at 12:14 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>
>>>>>
>>>>> On Jan 12, 2016, at 12:03 PM, Doug Simon <doug.simon at oracle.com> wrote:
>>>>>
>>>>>>
>>>>>> On 12 Jan 2016, at 22:39, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jan 12, 2016, at 10:14 AM, Doug Simon <doug.simon at oracle.com> wrote:
>>>>>>>
>>>>>>> If we’re going with an enum, you could put accessors directly in the enum:
>>>>>>>
>>>>>>> private static final boolean TrustFinalDefaultFields = Option.TrustFinalDefaultFields.getBoolean(true);
>>>>>>>
>>>>>>> private static final String TraceMethodDataFilter = Option.TraceMethodDataFilter.getString(null);
>>>>>>>
>>>>>>> You could then type the value of the options and check the right accessor is used:
>>>>>>>
>>>>>>> public enum Option {
>>>>>>> ImplicitStableValues(boolean.class),
>>>>>>> InitTimer, // Note: Not used because of visibility issues (see InitTimer.ENABLED).
>>>>>>> PrintConfig(boolean.class),
>>>>>>> PrintFlags(boolean.class),
>>>>>>> ShowFlags(boolean.class),
>>>>>>> TraceMethodDataFilter(String.class),
>>>>>>> TrustFinalDefaultFields(String.class);
>>>>>>>
>>>>>>> Even ignoring these suggestions, the discipline imposed by the enum if a good idea.
>>>>>>
>>>>>> Excellent idea! I was also thinking about adding the default value to the enum.
>>>>>
>>>>> Can you do that without having to box the default value?
>>>>
>>>> No, we have to box but we can initialize all flags in the constructor:
>>>>
>>>> http://cr.openjdk.java.net/~twisti/8146820/webrev.02/
>>
>> Do we agree on the change?
>
> I would prefer it if the value was lazy initialized (for non-AOT runtimes):
It’s not different in AOT-land because these cannot be constants.
>
>
> /**
> * Supported JVMCI options.
> */
> public enum Option {
> ImplicitStableValues(boolean.class, true),
> InitTimer(boolean.class, false), // Note: Not used (see InitTimer.ENABLED).
> PrintConfig(boolean.class, false),
> PrintFlags(boolean.class, false),
> ShowFlags(boolean.class, false),
> TraceMethodDataFilter(String.class, null),
> TrustFinalDefaultFields(String.class, true);
>
> /**
> * The prefix for system properties that are JVMCI options.
> */
> private static final String JVMCI_OPTION_PROPERTY_PREFIX = "jvmci.";
>
> private final Class<?> type;
> private Object value;
> private final Object defaultValue;
> private boolean isDefault;
>
> private Option(Class<?> type, Object defaultValue) {
> assert Character.isUpperCase(name().charAt(0)) : "Option name must start with upper-case letter: " + name();
> this.type = type;
> this.value = "UNINITIALIZED";
> this.defaultValue = defaultValue;
> }
>
> private Object getValue() {
> if (value == "UNINITIALIZED") {
> String propertyValue = VM.getSavedProperty(JVMCI_OPTION_PROPERTY_PREFIX + name());
> if (propertyValue == null) {
> this.value = defaultValue;
> this.isDefault = true;
> } else {
> if (type == boolean.class) {
> this.value = Boolean.parseBoolean(propertyValue);
> } else if (type == String.class) {
> this.value = propertyValue;
> } else {
> throw new JVMCIError("Unexpected option type " + type);
> }
> this.isDefault = false;
> }
> // Saved properties should not be interned - let’s be sure
> assert value != "UNINITIALIZED";
> }
> return value;
> }
>
> /**
> * Returns the option's value as boolean.
> *
> * @return option's value
> */
> public boolean getBoolean() {
> return (boolean) getValue();
> }
>
> /**
> * Returns the option's value as String.
> *
> * @return option's value
> */
> public String getString() {
> return (String) getValue();
> }
>
> /**
> * Prints all option flags to {@code out}.
> *
> * @param out stream to print to
> */
> public static void printFlags(PrintStream out) {
> out.println("[List of JVMCI options]");
> for (Option option : values()) {
> Object value = option.getValue();
> String assign = option.isDefault ? ":=" : " =";
> out.printf("%9s %-40s %s %-14s%n", option.type.getSimpleName(), option, assign, value);
> }
> }
> }
>
>
> Also, you can remove all the static fields that just cache a (possibly unboxed) option value and use the option directly. For example:
>
> diff -r 1034ff44c5d0 src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaFieldImpl.java
> --- a/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaFieldImpl.java Tue Jan 12 15:04:27 2016 +0100
> +++ b/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaFieldImpl.java Thu Jan 14 13:40:28 2016 +0100
> @@ -29,6 +29,7 @@
> import java.lang.reflect.Field;
>
> import jdk.vm.ci.common.JVMCIError;
> +import jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.Option;
> import jdk.vm.ci.meta.JavaType;
> import jdk.vm.ci.meta.LocationIdentity;
> import jdk.vm.ci.meta.MetaAccessProvider;
> @@ -41,11 +42,6 @@
> */
> class HotSpotResolvedJavaFieldImpl implements HotSpotResolvedJavaField, HotSpotProxified {
>
> - /**
> - * Mark well-known stable fields as such.
> - */
> - private static final boolean ImplicitStableValues = HotSpotJVMCIRuntime.getBooleanProperty("jvmci.ImplicitStableValues", true);
> -
> private final HotSpotResolvedObjectTypeImpl holder;
> private final String name;
> private JavaType type;
> @@ -198,7 +194,7 @@
> return true;
> }
> assert getAnnotation(Stable.class) == null;
> - if (ImplicitStableValues && isImplicitStableField()) {
> + if (Option.ImplicitStableValues.getBoolean() && isImplicitStableField()) {
> return true;
> }
> return false;
>
> None of the current options are used in tight loops where the cost of the unboxing (if any) would matter.
Right.
>
> Lastly, since you’ve added PrintFlags and ShowFlags, why not add a help message to each option. For example:
>
> ImplicitStableValues(boolean.class, true, “Mark well-known stable fields as such.”),
We should.
http://cr.openjdk.java.net/~twisti/8146820/webrev.03/
$ ./build/macosx-x86_64-normal-server-release/jdk/bin/java -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -Djvmci.ShowFlags=true InitGraal
[List of JVMCI options]
boolean ImplicitStableValues := true Mark well-known stable fields as such.
boolean InitTimer := false Specifies if initialization timing is enabled.
boolean PrintConfig := false Prints all HotSpotVMConfig fields.
boolean PrintFlags := false Prints all JVMCI flags and exits.
boolean ShowFlags = true Prints all JVMCI flags and continues.
String TraceMethodDataFilter := null
boolean TrustFinalDefaultFields := true Determines whether to treat final fields with default values as constant.
>
> -Doug
>
>>
>>>>
>>>> We will not have many flags so this should be alright. A PrintFlags looks like this:
>>>>
>>>> $ ./build/macosx-x86_64-normal-server-release/jdk/bin/java -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -Djvmci.PrintFlags=true InitGraal
>>>> [List of JVMCI options]
>>>> boolean ImplicitStableValues := true
>>>> boolean InitTimer := false
>>>> boolean PrintConfig := false
>>>> boolean PrintFlags = true
>>>> boolean ShowFlags := false
>>>> String TraceMethodDataFilter := null
>>>> String TrustFinalDefaultFields := true
>>>
>>> …and this is a bug, of course :-)
>>>
>>>>
>>>> I’m almost tempted to move InitTimer to another package, like jdk.vm.ci.common …
>>>>
>>>>>
>>>>> -Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160114/49aedf14/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list