PING: RFR: 8217845: SA should refer const values for JVMFlag from HotSpot

[serguei.spitsyn at oracle.com]

+1

Thanks,
Serguei


On 2/4/19 21:18, Jini George wrote:
> Looks ok to me. Thanks!
>
> -Jini.
>
> On 2/5/2019 10:25 AM, Yasumasa Suenaga wrote:
>> Hi Jini,
>>
>> Thank you for your comment!
>> I uploaded new webrev. Could you review again?
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8217845/webrev.01/
>>
>> Diff from previous webrev is here:
>>
>>    http://hg.openjdk.java.net/jdk/submit/rev/6e603980ab28
>>
>>
>> Yasumasa
>>
>> 2019年2月5日(火) 12:01 Jini George <jini.george at oracle.com>:
>>>
>>> Hi Yasumasa,
>>>
>>> Thanks a bunch for this welcome change. Looks good to me overall -- a
>>> few points though.
>>>
>>> 1. Nit: alignment:
>>>     180     // See JVMFlag::print_origin() in HotSpot
>>>
>>> 2. In test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java,
>>>
>>> 156             expStrMap.put("flags",
>>> 157                           List.of("command line", "ergonomic",
>>> "default"));
>>>
>>> I think you don't need to do one more round of testing by starting
>>> LingeredApp again and issuing the 'flags' command with
>>> runFlagOriginTest(). You should be able to check for the origin strings
>>> in runBasicTest() itself. You can add "command line", "ergonomic",
>>> "default" to
>>>
>>>    66             expStrMap.put("flags", List.of(
>>>    67                     "UnlockDiagnosticVMOptions = true",
>>>    68                     "MaxFDLimit = false",
>>>    69                     "MaxJavaStackTraceDepth = 1024",
>>>    70                     "VerifyMergedCPBytecodes",
>>>    71                     "ConcGCThreads", "UseThreadPriorities",
>>>    72                     "ShowHiddenFrames"));
>>>
>>> You can have it like this:
>>>
>>>    65             Map<String, List<String>> expStrMap = new 
>>> HashMap<>();
>>>    66             expStrMap.put("flags", List.of(
>>>    67                     "command line", "ergonomic", "default",
>>>    68                     "UnlockDiagnosticVMOptions = true",
>>>    69                     "MaxFDLimit = false",
>>>    70                     "MaxJavaStackTraceDepth = 1024",
>>>    71                     "VerifyMergedCPBytecodes",
>>>    72                     "ConcGCThreads", "UseThreadPriorities",
>>>    73                     "ShowHiddenFrames"));
>>>
>>> Thank you,
>>> Jini.
>>>
>>>
>>> On 2/4/2019 6:00 PM, Yasumasa Suenaga wrote:
>>>> PING: Could you review it?
>>>>
>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8217845
>>>>>>     webrev: 
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8217845/webrev.00/
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2019/01/28 9:35, Yasumasa Suenaga wrote:
>>>>> Hi all,
>>>>>
>>>>> This change has passed tests as below (all of jhsdb related tests):
>>>>>
>>>>>    - Submit repo
>>>>>    - All hotspot/jtreg/serviceability/sa
>>>>>    - 
>>>>> hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java
>>>>>    - All jdk/sun/tools/jhsdb
>>>>>    - jdk/tools/launcher/HelpFlagsTest.java
>>>>>
>>>>>
>>>>> Comments are welcome.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2019/01/26 13:43, Yasumasa Suenaga wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this change:
>>>>>>
>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8217845
>>>>>>     webrev: 
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8217845/webrev.00/
>>>>>>
>>>>>> SA will handle `Flags` enums to get flag origin.
>>>>>> However SA has const value for bitmask for flag, and shows as raw
>>>>>> (int) value.
>>>>>> This issue is commented in [1].
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> [1]
>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/8c035b34248d/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java#l166 
>>>>>>
>>>>>>