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.
