RFR (S): 8146820: JVMCI properties should use HotSpotJVMCIRuntime.getBooleanProperty mechanism
Doug Simon
doug.simon at oracle.com
Thu Jan 14 12:44:42 UTC 2016
> 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):
/**
* 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.
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."),
-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
More information about the hotspot-compiler-dev
mailing list