RFR: JDK-8177763: Getting an hprof dump via jcmd could benefit from stronger option checking

Wed Aug 29 19:49:40 UTC 2018

On 8/29/18 12:42 PM, gary.adams at oracle.com wrote:
> On 8/29/18 3:12 PM, Chris Plummer wrote:
>> On 8/29/18 11:57 AM, Gary Adams wrote:
>>> Here are the items I'd like to discuss.
>>>
>>>    1. The current help doc only mention <filename>
>>>        as a positional argument.
>>>        The original bug was posted with an expectation
>>>         that <filename=value> should be accepted.
>>>        Do we document that we support either form
>>>        in the help message?
>> That's a good question. Or should we even support both forms. I think 
>> other dcmds require filename=<filename>. If we decide to not use your 
>> new support for positional arguments specified with key=<value>, then 
>> we should do a better job of detecting when this mistake is made and 
>> give an appropriate error. I think currently (without your changes) 
>> if you specify filename=foo,  you get a file named "filename=foo".
> Actually, you get the file named "filename".
> The dcmd parsing already split the "key=value" parameter,
> but since only a positional argument was expected it took
> the <key> portion.
>
> I like supporting either form of the argument.
> It's backward compatible if people are already using it correctly.
> And it's forward compatible with the syntax of the JFR key=value
> expectations.
Maybe we should just document the preferred way.
>
>>> Do we change the issue type from bug to rfe?
>> Not sure. I suppose confusing argument handling could be considered a 
>> bug.
> Actually it's not confusing. It matches what the help text describes.
> The confusion was introduced when the JFR commands introduced
> a different approach to parameters.
JFR is key=value and we didn't have that previously?

Chris
>
>>> Do we need a CSR if the dcmd syntax is changed?
>> I think yes.
>>>
>>>   2. Do we have a native function that will declare a
>>>       path as absolute or relative to the current directory?
>>>       Worried a little about windows paths here.
>> I don't know. There probably is one to canonicalize a path.
>>
>> Chris
>>>
>>>   3. Looks like minimal heap_dump testing here:
>>> open/test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpTest.java
>>> ...
>>>
>>> On 8/29/18, 2:04 PM, Chris Plummer wrote:
>>>> Hi Gary,
>>>>
>>>> Ok. Overall looks good. A couple of comments:
>>>>
>>>> You now output the path of the created file, but in a kind of 
>>>> strange way. You print the CWD, and then the specified path, 
>>>> whether relative or full. So it's kind of confusing because if the 
>>>> user specifies a full path, they'll still see the CWD printed out. 
>>>> Why not just print the full path (after combining with CWD if 
>>>> needed), or maybe omit the CWD off when a full path was specified. 
>>>> If determining if CWD ends up getting used is difficult, maybe just 
>>>> make it more clear what you are printing is the CWD, and it is not 
>>>> necessarily part of the actual filename path.
>>>>
>>>> What about writing some tests cases. It would probably be pretty 
>>>> easy to update some of the existing test cases to use key=<value> 
>>>> where previously they just allowed <value>.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>>
>>>> On 8/29/18 8:10 AM, Gary Adams wrote:
>>>>> Sure no problem.
>>>>>
>>>>> I have two workspaces I use called jdk-jdb and jdk-jdk.
>>>>> I have the changes in the jdk-jdb workspace for the heap
>>>>> filename updates.
>>>>>
>>>>> In the jdk-jdb workspace a launch a process that just waits for 
>>>>> input.
>>>>>
>>>>>    jdk/bin/java -cp ~/classes my
>>>>>
>>>>> In the jdk-jdk workspace I issue the jcmd to talk to the java process
>>>>> running in the jdk-jdb workspace. The updated output includes
>>>>> "[<current-directory>,<heap dump filename>]"
>>>>>
>>>>> ...
>>>>>
>>>>> $  jdk/bin/jcmd 2328 GC.heap_dump filename=foo
>>>>> 2328:
>>>>> Heap dump file created 
>>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,foo]
>>>>>
>>>>> $  jdk/bin/jcmd 2328 GC.heap_dump filename
>>>>> 2328:
>>>>> Heap dump file created 
>>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,filename]
>>>>>
>>>>> $  jdk/bin/jcmd 2328 GC.heap_dump ffoo2
>>>>> 2328:
>>>>> Heap dump file created 
>>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,ffoo2]
>>>>>
>>>>> $  jdk/bin/jcmd 2328 GC.heap_dump ffoo2
>>>>> 2328:
>>>>> File exists [/export/users/gradams/ws/jdk-jdb/build/linux-x64,ffoo2]
>>>>>
>>>>> $  jdk/bin/jcmd 2328 GC.heap_dump ~/ffoo2
>>>>> 2328:
>>>>> Heap dump file created 
>>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,/home/gradams/ffoo2] 
>>>>>
>>>>>
>>>>> $  jdk/bin/jcmd 2328 GC.heap_dump ~/ffoo2
>>>>> 2328:
>>>>> File exists 
>>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,/home/gradams/ffoo2] 
>>>>>
>>>>>
>>>>> $  jdk/bin/jcmd 2328 GC.heap_dump /tmp/ffoo2
>>>>> 2328:
>>>>> Heap dump file created 
>>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,/tmp/ffoo2]
>>>>>
>>>>> $  jdk/bin/jcmd 2328 GC.heap_dump /tmp/ffoo2
>>>>> 2328:
>>>>> File exists 
>>>>> [/export/users/gradams/ws/jdk-jdb/build/linux-x64,/tmp/ffoo2]
>>>>>
>>>>> $ pwd
>>>>> /home/gradams/scratch/ws/jdk-jdk/build/linux-x64
>>>>>
>>>>>
>>>>> On 8/29/18, 2:21 AM, Chris Plummer wrote:
>>>>>> Hi Gary,
>>>>>>
>>>>>> What would be really useful are some before/after examples that 
>>>>>> demonstrate what has changed from the users point of view.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 8/28/18 5:26 AM, Gary Adams wrote:
>>>>>>> This message may have been lost in the vacation email backlog, 
>>>>>>> so I'm
>>>>>>> sending it again.
>>>>>>>
>>>>>>> On 8/9/18, 2:19 PM, Gary Adams wrote:
>>>>>>>> Thee are several reported problems using jcmd for obtaining 
>>>>>>>> heap dumps.
>>>>>>>>
>>>>>>>> Some users are confused by the positional argument for a filename
>>>>>>>> when a similar jfr command uses "key=value" syntax.
>>>>>>>>
>>>>>>>> Some users are confused by the basic nature of the command, 
>>>>>>>> where the jvm
>>>>>>>> executing the heap dump is running in a different current 
>>>>>>>> directory than
>>>>>>>> the jcmd itself.
>>>>>>>>
>>>>>>>> This is a proposed fix to report the current directory and the 
>>>>>>>> filename
>>>>>>>> when the heap dump is successfully written or if an error 
>>>>>>>> occurs. There
>>>>>>>> is also a proposed fix to handle the case when the user provided
>>>>>>>> "key=value" inputs. If the key matches the argument name, then the
>>>>>>>> user intended the value to be used.
>>>>>>>>
>>>>>>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8177763
>>>>>>>>   Webrev: http://cr.openjdk.java.net/~gadams/8177763/webrev.00/
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>