[10] RFR: 8186018: SA: Monitor Cache Dump in HSDB does not work

[David Holmes]

On 9/08/2017 5:52 PM, Yasumasa Suenaga wrote:
> Hi David,
> 
>> Copyright format is just two years: first and last, so should be:
>>
>> * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights
>> reserved.
> 
> Sorry, I've fixed:
> 
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8186018/webrev.02/
> 
> 
> Could you push?

I'll wait for second review. If no one volunteers "overnight" I'll push 
in my morning.

Cheers,
David

> 
> Thanks,
> 
> Yasumasa
> 
> 
> 2017-08-09 16:40 GMT+09:00 David Holmes <david.holmes at oracle.com>:
>> Hi,
>>
>> On 9/08/2017 4:55 PM, Yasumasa Suenaga wrote:
>>>
>>> Hi David,
>>>
>>> I uploaded new webrev:
>>>
>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8186018/webrev.01/
>>>
>>> 2017-08-09 15:32 GMT+09:00 David Holmes <david.holmes at oracle.com>:
>>>>
>>>> Hi Yasumasa,
>>>>
>>>> On 9/08/2017 3:54 PM, Yasumasa Suenaga wrote:
>>>>>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> I tried to check Monitor Cache Dump in HSDB, but it hangs and OOME
>>>>> occurred.
>>>>>
>>>>> How to reproduce:
>>>>>
>>>>>      1. Run JShell
>>>>>      2. Attach HSDB to JShell
>>>>>         $ jhsdb hsdb --pid <PID>
>>>>>      3. Select "Monitor Cache Dump" in "Tools" menu.
>>>>>
>>>>> ObjectSynchronizer$ObjectMonitorIterator switches monitor block list
>>>>> when index == 0 at next(). However next() switches blockAddr only.
>>>>> We should also switch "block".
>>>>>
>>>>> I uloaded webrev for this issue. Could you review it?
>>>>>
>>>>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8186018/webrev.00/
>>>>
>>>>
>>>>
>>>> The fix looks good. Can you please add a comment inserted at L112:
>>>>
>>>> // advance to next block
>>>>
>>>> Also update copyright year.
>>>
>>>
>>> Fixed them.
>>
>>
>> Copyright format is just two years: first and last, so should be:
>>
>> * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights
>> reserved.
>>
>>>
>>>> Not sure if we need to do anything explicit about calling next() when
>>>> we've
>>>> reached the end of the last block? Current code will throw NPE at L116,
>>>> new
>>>> code will either throw NPE at L116 or perhaps at L113 if ObjectMonitor
>>>> constructor doesn't take null.
>>>
>>>
>>> According to JavaDoc [1], next() throws NoSuchElementException if the
>>> iterator has no more elements.
>>> So I modified to throw this exception if blockAddr is null.
>>
>>
>> That looks good to me.
>>
>> Thanks,
>> David
>>
>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> [1]
>>> https://docs.oracle.com/javase/8/docs/api/java/util/Iterator.html#next--
>>>
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>
>>