[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8156033: jhsdb jmap cannot set heapdump name
From:       Dmitry Samersoff <dmitry.samersoff () oracle ! com>
Date:       2016-05-06 15:36:44
Message-ID: 572CBA0C.70509 () oracle ! com
[Download RAW message or body]

Yasumasa,

Looks good for me.

Code below is not necessary - this test is not intended to deal with
possible dump format error, just check that file with correct name is
created. Please remove it (no need to re-review).

BasicLauncherTest.java:

 151         try {
 152             HprofParser.parse(dump);
 153         } catch (Exception e) {
 154             e.printStackTrace();
 155             fail("Could not parse dump file " +
dump.getAbsolutePath());
 156         }

-Dmitry


On 2016-05-06 17:41, Yasumasa Suenaga wrote:
> Hi Dmitry,
> 
> Thank you for your comment.
> I uploaded new webrev. Could you review again?
> 
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8156033/webrev.01/hotspot/
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8156033/webrev.01/jdk/
> 
> BasicLauncherTest works fine.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2016/05/06 21:46, Dmitry Samersoff wrote:
>> Yasumasa,
>>
>>> BasicLauncherTest launches each SA tools with --pid argument only.
>>> Should we add test for generating heapdump in it?
>>
>> Yes.
>>
>> Please  make sure that LingeredApp is launched with small heap value
>> (-Xmx256M) at ll. 103
>>
>> and add something like
>>
>> launch("<some string>", "jmap", "--binaryheap",
>>        "--dumpfile=<filename>");
>>
>> -Dmitry
>>
>> On 2016-05-06 15:24, Yasumasa Suenaga wrote:
>>> Hi Dmitry,
>>>
>>> Thank you for reviewing!
>>>
>>> On 2016/05/06 21:06, Dmitry Samersoff wrote:
>>>> Yasumasa,
>>>>
>>>> The changes looks good. I'll sponsor the push.
>>>
>>> Thanks!
>>>
>>>
>>>> 1. I would prefer to check arguments explicitly for better readability:
>>>>
>>>> if (!requestHeapDump && dumpfile != null) {
>>>>     throw IAE("Unexpected argument dumpfile");
>>>> }
>>>
>>> I'll fix it.
>>>
>>>
>>>> 2. Please update
>>>>
>>>> jdk/test/sun/tools/jhsdb/BasicLauncherTest.java
>>>
>>> BasicLauncherTest launches each SA tools with --pid argument only.
>>> Should we add test for generating heapdump in it?
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>>> -Dmitry
>>>>
>>>> On 2016-05-04 17:47, Yasumasa Suenaga wrote:
>>>>> Hi all,
>>>>>
>>>>> This review request relates to [1].
>>>>>
>>>>> We cannot change heapdump name (heap.bin) when we genarate heapdump
>>>>> via
>>>>> jhsdb.
>>>>>
>>>>> When we use jmap, we can change heapdump name through file option.
>>>>> I want to set it like a jmap.
>>>>>
>>>>> I uploaded webrev for this enhancement.
>>>>> Could you review it?
>>>>>
>>>>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8156033/webrev.00/
>>>>>
>>>>> I cannot access JPRT.
>>>>> So I need a sponsor.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> [1]
>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-April/019510.html
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
[prev in list] [next in list] [prev in thread] [next in thread]