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

[David Holmes]

Thanks Dan.

Yasumasa, I'm pushing the changes.

David

On 10/08/2017 2:52 AM, Daniel D. Daugherty wrote:

On 8/9/17 3:14 AM, David Holmes wrote:

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/


src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectSynchronizer.java 


 No comments.

Thumbs up.

Dan





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 :

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 :


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 
 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

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

[Daniel D. Daugherty]

On 8/9/17 3:14 AM, David Holmes wrote:

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/


src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectSynchronizer.java
No comments.

Thumbs up.

Dan





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 :

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 :


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 
 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

Re: [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 :

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 :


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 
 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

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

[Yasumasa Suenaga]

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?


Thanks,

Yasumasa


2017-08-09 16:40 GMT+09:00 David Holmes :
> 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 :
>>>
>>> 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 
 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

>>>
>

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

[David Holmes]

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 :

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 
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

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

[Yasumasa Suenaga]

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 :
> 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 
>>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.


> 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.


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
>>
>

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

[David Holmes]

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 
   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.

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.


Thanks,
David


I cannot access JPRT. So I need a sponsor.


Thanks,

Yasumasa

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

[Yasumasa Suenaga]

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 
  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/

I cannot access JPRT. So I need a sponsor.


Thanks,

Yasumasa