RFR (S): 8146820: JVMCI properties should use HotSpotJVMCIRuntime.getBooleanProperty mechanism

[Christian Thalinger]

Thanks, Doug.

> On Jan 14, 2016, at 11:55 AM, Doug Simon <doug.simon at oracle.com> wrote:
> 
> Looks good.
> 
>> On 14 Jan 2016, at 22:50, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>> 
>>> 
>>> 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.
>> 
>>> 
>>> 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.
>