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

[Christian Thalinger]

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

> 
> -Doug
> 
> 
>> On 12 Jan 2016, at 21:04, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>> 
>>> 
>>> On Jan 11, 2016, at 12:51 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>> 
>>> 
>>>> On Jan 11, 2016, at 10:14 AM, Doug Simon <doug.simon at oracle.com> wrote:
>>>> 
>>>> 
>>>>> On 11 Jan 2016, at 20:50, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>> 
>>>>> What is naming convention for properties?
>>>>> Do we have somewhere list of all JVMCI properties we accept? May be we should add it.
>>>> 
>>>> Currently, there is no list of accepted JVMCI properties. Once Chris applies the changes below such that all system property access (apart from jvmci.InitTimer) goes through HotSpotJVMCIRuntime.getProperty(), then the javadoc of that method could contain the list (much like System.getProperties describes the supported standard properties).
>>> 
>>> Good idea.
>>> 
>>>> 
>>>>> All JVMCI properties names should be consistent whatever you choose.
>>>> 
>>>> I agree.
>>> 
>>> Yes.  They should feel like our other command line options so camel-case is what I had in mind.
>> 
>> How about this:
>> 
>> http://cr.openjdk.java.net/~twisti/8146820/webrev.01/index.html
>> 
>> Now all options are in an enum so that we can have PrintFlags and ShowFlags options.  I did not add any documentation but we could.
>> 
>>> 
>>>> 
>>>> -Doug
>>>> 
>>>>> 
>>>>> 'inittimer' is also lowcased.
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> On 1/11/16 11:15 AM, Christian Thalinger wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8146820
>>>>>> 
>>>>>> I’ve renamed traceMethodDataFilter to TraceMethodDataFilter.  Should we rename printconfig to PrintConfig?
>>>>>> 
>>>>>> diff -r c90679b0ea25 src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
>>>>>> --- a/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java	Fri Dec 18 20:23:28 2015 +0300
>>>>>> +++ b/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java	Mon Jan 11 09:12:48 2016 -1000
>>>>>> @@ -85,6 +85,21 @@ public final class HotSpotJVMCIRuntime i
>>>>>>  }
>>>>>> 
>>>>>>  /**
>>>>>> +     * Gets a String value based on a system property {@linkplain VM#getSavedProperty(String) saved}
>>>>>> +     * at system initialization time. The property name is prefixed with "{@code jvmci.}".
>>>>>> +     *
>>>>>> +     * @param name the name of the system property
>>>>>> +     * @param def the value to return if there is no system property corresponding to {@code name}
>>>>>> +     */
>>>>>> +    public static String getProperty(String name, String def) {
>>>>>> +        String value = VM.getSavedProperty("jvmci." + name);
>>>>>> +        if (value == null) {
>>>>>> +            return def;
>>>>>> +        }
>>>>>> +        return value;
>>>>>> +    }
>>>>>> +
>>>>>> +    /**
>>>>>>   * Gets a boolean value based on a system property {@linkplain VM#getSavedProperty(String)
>>>>>>   * saved} at system initialization time. The property name is prefixed with "{@code jvmci.}".
>>>>>>   *
>>>>>> @@ -93,7 +108,7 @@ public final class HotSpotJVMCIRuntime i
>>>>>>   * @param def the value to return if there is no system property corresponding to {@code name}
>>>>>>   */
>>>>>>  public static boolean getBooleanProperty(String name, boolean def) {
>>>>>> -        String value = VM.getSavedProperty("jvmci." + name);
>>>>>> +        String value = getProperty(name, null);
>>>>>>      if (value == null) {
>>>>>>          return def;
>>>>>>      }
>>>>>> @@ -164,7 +179,7 @@ public final class HotSpotJVMCIRuntime i
>>>>>>      }
>>>>>>      metaAccessContext = context;
>>>>>> 
>>>>>> -        if (Boolean.valueOf(System.getProperty("jvmci.printconfig"))) {
>>>>>> +        if (getBooleanProperty("printconfig", false)) {
>>>>>>          printConfig(config, compilerToVm);
>>>>>>      }
>>>>>> 
>>>>>> diff -r c90679b0ea25 src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java
>>>>>> --- a/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java	Fri Dec 18 20:23:28 2015 +0300
>>>>>> +++ b/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java	Mon Jan 11 09:12:48 2016 -1000
>>>>>> @@ -417,7 +417,7 @@ final class HotSpotResolvedJavaMethodImp
>>>>>>      return false;
>>>>>>  }
>>>>>> 
>>>>>> -    private static final String TraceMethodDataFilter = System.getProperty("jvmci.traceMethodDataFilter");
>>>>>> +    private static final String TraceMethodDataFilter = HotSpotJVMCIRuntime.getProperty("TraceMethodDataFilter", null);
>>>>>> 
>>>>>>  @Override
>>>>>>  public ProfilingInfo getProfilingInfo(boolean includeNormal, boolean includeOSR) {
>>>>>> diff -r c90679b0ea25 src/jdk.vm.ci/share/classes/jdk.vm.ci.inittimer/src/jdk/vm/ci/inittimer/InitTimer.java
>>>>>> --- a/src/jdk.vm.ci/share/classes/jdk.vm.ci.inittimer/src/jdk/vm/ci/inittimer/InitTimer.java	Fri Dec 18 20:23:28 2015 +0300
>>>>>> +++ b/src/jdk.vm.ci/share/classes/jdk.vm.ci.inittimer/src/jdk/vm/ci/inittimer/InitTimer.java	Mon Jan 11 09:12:48 2016 -1000
>>>>>> @@ -65,9 +65,11 @@ public final class InitTimer implements
>>>>>>  }
>>>>>> 
>>>>>>  /**
>>>>>> -     * Specifies if initialization timing is enabled.
>>>>>> +     * Specifies if initialization timing is enabled. Note: this property cannot use
>>>>>> +     * {@code HotSpotJVMCIRuntime.getBooleanProperty} since that class is not visible from this
>>>>>> +     * package.
>>>>>>   */
>>>>>> -    private static final boolean ENABLED = Boolean.getBoolean("jvmci.inittimer") || Boolean.getBoolean("jvmci.runtime.TimeInit");
>>>>>> +    private static final boolean ENABLED = Boolean.getBoolean("jvmci.inittimer");
>>>>>> 
>>>>>>  public static final AtomicInteger nesting = ENABLED ? new AtomicInteger() : null;
>>>>>>  public static final String SPACES = "                                            ";
>