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

[Christian Thalinger]

> 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.