RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Yasumasa Suenaga-4
Hi all,

This review request is a part of [1].


JBS:
  https://bugs.openjdk.java.net/browse/JDK-8187403

webrev:
  http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/


I cannot access JPRT. So I need a sponsor.


Thanks,

Yasumasa


[1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
Reply | Threaded
Open this post in threaded view
|

PING: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Yasumasa Suenaga-4
PING:

Have you checked this issue?

>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/


Yasumasa


On 2017/09/11 11:18, Yasumasa Suenaga wrote:

> Hi all,
>
> This review request is a part of [1].
>
>
> JBS:
>    https://bugs.openjdk.java.net/browse/JDK-8187403
>
> webrev:
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>
>
> I cannot access JPRT. So I need a sponsor.
>
>
> Thanks,
>
> Yasumasa
>
>
> [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>
Reply | Threaded
Open this post in threaded view
|

RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Yasumasa Suenaga-4
Hi all,

I uploaded new webrev to be adapted to jdk10/hs:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/


Thanks,

Yasumasa


On 2017/09/21 7:48, Yasumasa Suenaga wrote:

> PING:
>
> Have you checked this issue?
>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>
>
> Yasumasa
>
>
> On 2017/09/11 11:18, Yasumasa Suenaga wrote:
>> Hi all,
>>
>> This review request is a part of [1].
>>
>>
>> JBS:
>>    https://bugs.openjdk.java.net/browse/JDK-8187403
>>
>> webrev:
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>
>>
>> I cannot access JPRT. So I need a sponsor.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Yasumasa Suenaga-4
Hi all,

I added noreg-hard label to JBS because this issue appears Stack
Memory window on HSDB (GUI application). So it is hard to test.

  https://bugs.openjdk.java.net/browse/JDK-8187403


Thanks,

Yasumasa



2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <[hidden email]>:

> Hi all,
>
> I uploaded new webrev to be adapted to jdk10/hs:
>
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2017/09/21 7:48, Yasumasa Suenaga wrote:
>>
>> PING:
>>
>> Have you checked this issue?
>>
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>
>>
>>
>> Yasumasa
>>
>>
>>
>> On 2017/09/11 11:18, Yasumasa Suenaga wrote:
>>>
>>> Hi all,
>>>
>>> This review request is a part of [1].
>>>
>>>
>>> JBS:
>>>    https://bugs.openjdk.java.net/browse/JDK-8187403
>>>
>>> webrev:
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>>
>>>
>>> I cannot access JPRT. So I need a sponsor.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> [1]
>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

serguei.spitsyn@oracle.com
Hi Yasumasa,

Could you, please, also add some evaluation to the bug report about what
is the root cause and how do you fix it?

Thanks,
Serguei


On 9/26/17 18:10, Yasumasa Suenaga wrote:

> Hi all,
>
> I added noreg-hard label to JBS because this issue appears Stack
> Memory window on HSDB (GUI application). So it is hard to test.
>
>    https://bugs.openjdk.java.net/browse/JDK-8187403
>
>
> Thanks,
>
> Yasumasa
>
>
>
> 2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <[hidden email]>:
>> Hi all,
>>
>> I uploaded new webrev to be adapted to jdk10/hs:
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2017/09/21 7:48, Yasumasa Suenaga wrote:
>>> PING:
>>>
>>> Have you checked this issue?
>>>
>>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>>
>>>
>>> Yasumasa
>>>
>>>
>>>
>>> On 2017/09/11 11:18, Yasumasa Suenaga wrote:
>>>> Hi all,
>>>>
>>>> This review request is a part of [1].
>>>>
>>>>
>>>> JBS:
>>>>     https://bugs.openjdk.java.net/browse/JDK-8187403
>>>>
>>>> webrev:
>>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>>>
>>>>
>>>> I cannot access JPRT. So I need a sponsor.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> [1]
>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Yasumasa Suenaga-4
Hi Serguei,

I added it to JBS:
  https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248

Sorry for my English. I'm not good at English...


Yasumasa



2017-09-29 8:27 GMT+09:00 [hidden email]
<[hidden email]>:

> Hi Yasumasa,
>
> Could you, please, also add some evaluation to the bug report about what is
> the root cause and how do you fix it?
>
> Thanks,
> Serguei
>
>
>
> On 9/26/17 18:10, Yasumasa Suenaga wrote:
>>
>> Hi all,
>>
>> I added noreg-hard label to JBS because this issue appears Stack
>> Memory window on HSDB (GUI application). So it is hard to test.
>>
>>    https://bugs.openjdk.java.net/browse/JDK-8187403
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>
>> 2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <[hidden email]>:
>>>
>>> Hi all,
>>>
>>> I uploaded new webrev to be adapted to jdk10/hs:
>>>
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2017/09/21 7:48, Yasumasa Suenaga wrote:
>>>>
>>>> PING:
>>>>
>>>> Have you checked this issue?
>>>>
>>>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>>>
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>>
>>>> On 2017/09/11 11:18, Yasumasa Suenaga wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> This review request is a part of [1].
>>>>>
>>>>>
>>>>> JBS:
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8187403
>>>>>
>>>>> webrev:
>>>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>>>>
>>>>>
>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> [1]
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

serguei.spitsyn@oracle.com
Hi Yasumasa,

Just some minor comments.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java.frames.html

I'd suggest to make the lines 144-145 a one-liner.
It won't be that big. Otherwise, the indent is not right.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html

  The same as above for lines 85-86.
  It seems, there is no reason for renaming 'type' to 't' in the initialize() method. 


http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html

  89     public HeapRegion addrToRegion(Address addr) {
  90       return regions().getByAddress(addr);
  91     }

  A suggestion: replace 'addrToRegion' with 'getByAddress'.
  It will look similar to the 'heapRegionIterator.'

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html

  41     private static int freeTag;
  42 
  43     private static int youngMask;
  44 
  45     private static int humongousMask;
  46 
  47     private static int pinnedMask;
  48 
  49     private static int oldMask;
  50 
  51     private static CIntegerField tagField;
  Unneeded empty lines.

  Also, it looks like the fields 'freeTag' and 'pinnedMask' are never initialized.
  Not sure, if it is intentional.

Otherwise, the fix looks good to me.

Thanks,
Serguei


On 9/28/17 18:20, Yasumasa Suenaga wrote:
Hi Serguei,

I added it to JBS:
  https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248

Sorry for my English. I'm not good at English...

Please, don't worry about your English.
Your description looks good.
Thank you for the bug report update!

Thanks,
Serguei

Yasumasa



2017-09-29 8:27 GMT+09:00 [hidden email]
[hidden email]:
Hi Yasumasa,

Could you, please, also add some evaluation to the bug report about what is
the root cause and how do you fix it?

Thanks,
Serguei



On 9/26/17 18:10, Yasumasa Suenaga wrote:
Hi all,

I added noreg-hard label to JBS because this issue appears Stack
Memory window on HSDB (GUI application). So it is hard to test.

   https://bugs.openjdk.java.net/browse/JDK-8187403


Thanks,

Yasumasa



2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga [hidden email]:
Hi all,

I uploaded new webrev to be adapted to jdk10/hs:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/


Thanks,

Yasumasa


On 2017/09/21 7:48, Yasumasa Suenaga wrote:
PING:

Have you checked this issue?

    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/


Yasumasa



On 2017/09/11 11:18, Yasumasa Suenaga wrote:
Hi all,

This review request is a part of [1].


JBS:
    https://bugs.openjdk.java.net/browse/JDK-8187403

webrev:
    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/


I cannot access JPRT. So I need a sponsor.


Thanks,

Yasumasa


[1]

http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html


      

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Yasumasa Suenaga-4
Hi Serguet,

Thank you for your comment.

> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html
>
>   It seems, there is no reason for renaming 'type' to 't' in the
> initialize() method.

I added new private member "type" as HeapRegionType.
  http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.udiff.html

So I renamed to "t" to avoid conflict.


> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html
>
>   89     public HeapRegion addrToRegion(Address addr) {
>   90       return regions().getByAddress(addr);
>   91     }
>
>   A suggestion: replace 'addrToRegion' with 'getByAddress'.
>   It will look similar to the 'heapRegionIterator.'

I've implemented it to follow HotSpot implementation.
  http://hg.openjdk.java.net/jdk10/hs/file/3a45532a1854/src/hotspot/share/gc/g1/heapRegionManager.inline.hpp#l32

I think current proposal is easy to understand if other people check
this with HotSpot.
Should I rename to "getByAddress" ?


Other your comment will be fixed in new webrev later.

Thanks,

Yasumasa


2017-09-29 14:35 GMT+09:00 [hidden email]
<[hidden email]>:

> Hi Yasumasa,
>
> Just some minor comments.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java.frames.html
>
> I'd suggest to make the lines 144-145 a one-liner.
> It won't be that big. Otherwise, the indent is not right.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html
>
>   The same as above for lines 85-86.
>   It seems, there is no reason for renaming 'type' to 't' in the
> initialize() method.
>
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html
>
>   89     public HeapRegion addrToRegion(Address addr) {
>   90       return regions().getByAddress(addr);
>   91     }
>
>   A suggestion: replace 'addrToRegion' with 'getByAddress'.
>   It will look similar to the 'heapRegionIterator.'
>
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html
>
>   41     private static int freeTag;
>   42
>   43     private static int youngMask;
>   44
>   45     private static int humongousMask;
>   46
>   47     private static int pinnedMask;
>   48
>   49     private static int oldMask;
>   50
>   51     private static CIntegerField tagField;
>
>   Unneeded empty lines.
>
>   Also, it looks like the fields 'freeTag' and 'pinnedMask' are never
> initialized.
>   Not sure, if it is intentional.
>
> Otherwise, the fix looks good to me.
>
> Thanks,
> Serguei
>
>
> On 9/28/17 18:20, Yasumasa Suenaga wrote:
>
> Hi Serguei,
>
> I added it to JBS:
>
> https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248
>
> Sorry for my English. I'm not good at English...
>
>
> Please, don't worry about your English.
> Your description looks good.
> Thank you for the bug report update!
>
> Thanks,
> Serguei
>
>
> Yasumasa
>
>
>
> 2017-09-29 8:27 GMT+09:00 [hidden email]
> <[hidden email]>:
>
> Hi Yasumasa,
>
> Could you, please, also add some evaluation to the bug report about what is
> the root cause and how do you fix it?
>
> Thanks,
> Serguei
>
>
>
> On 9/26/17 18:10, Yasumasa Suenaga wrote:
>
> Hi all,
>
> I added noreg-hard label to JBS because this issue appears Stack
> Memory window on HSDB (GUI application). So it is hard to test.
>
>    https://bugs.openjdk.java.net/browse/JDK-8187403
>
>
> Thanks,
>
> Yasumasa
>
>
>
> 2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <[hidden email]>:
>
> Hi all,
>
> I uploaded new webrev to be adapted to jdk10/hs:
>
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2017/09/21 7:48, Yasumasa Suenaga wrote:
>
> PING:
>
> Have you checked this issue?
>
>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>
>
> Yasumasa
>
>
>
> On 2017/09/11 11:18, Yasumasa Suenaga wrote:
>
> Hi all,
>
> This review request is a part of [1].
>
>
> JBS:
>     https://bugs.openjdk.java.net/browse/JDK-8187403
>
> webrev:
>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>
>
> I cannot access JPRT. So I need a sponsor.
>
>
> Thanks,
>
> Yasumasa
>
>
> [1]
>
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

serguei.spitsyn@oracle.com
Hi Yasumasa,


On 9/28/17 23:21, Yasumasa Suenaga wrote:
Hi Serguet,

Thank you for your comment.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html

  It seems, there is no reason for renaming 'type' to 't' in the
initialize() method.
I added new private member "type" as HeapRegionType.
  http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.udiff.html

So I renamed to "t" to avoid conflict.

There is no conflict.
Only local is used in the initialize() method.
Also, the initialize() method is static so that the instance field 'type' is not in its scope.
Otherwise, you could use this.type to avoid such a conflict.


http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html

  89     public HeapRegion addrToRegion(Address addr) {
  90       return regions().getByAddress(addr);
  91     }

  A suggestion: replace 'addrToRegion' with 'getByAddress'.
  It will look similar to the 'heapRegionIterator.'
I've implemented it to follow HotSpot implementation.
  http://hg.openjdk.java.net/jdk10/hs/file/3a45532a1854/src/hotspot/share/gc/g1/heapRegionManager.inline.hpp#l32

I think current proposal is easy to understand if other people check
this with HotSpot.
Should I rename to "getByAddress" ?

  81     public Iterator<HeapRegion> heapRegionIterator() {
  82         return regions().heapRegionIterator(length());
  83     }
 . . .
  89     public HeapRegion addrToRegion(Address addr) {
  90       return regions().getByAddress(addr);
  91     }

  There is already regions().getByAddress(addr), so I'm suggesting to follow this local pattern.
  Renaming '
addrToRegion' to 'getByAddress' will unify it with the 'heapRegionIterator()'.
  Otherwise, you would need to rename 'getByAddress' to 'addrToRegion' everywhere.
  But I guess, it is better to avoid.

Thanks,
Serguei


Other your comment will be fixed in new webrev later.

Thanks,

Yasumasa


2017-09-29 14:35 GMT+09:00 [hidden email]
[hidden email]:
Hi Yasumasa,

Just some minor comments.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java.frames.html

I'd suggest to make the lines 144-145 a one-liner.
It won't be that big. Otherwise, the indent is not right.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html

  The same as above for lines 85-86.
  It seems, there is no reason for renaming 'type' to 't' in the
initialize() method.


http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html

  89     public HeapRegion addrToRegion(Address addr) {
  90       return regions().getByAddress(addr);
  91     }

  A suggestion: replace 'addrToRegion' with 'getByAddress'.
  It will look similar to the 'heapRegionIterator.'


http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html

  41     private static int freeTag;
  42
  43     private static int youngMask;
  44
  45     private static int humongousMask;
  46
  47     private static int pinnedMask;
  48
  49     private static int oldMask;
  50
  51     private static CIntegerField tagField;

  Unneeded empty lines.

  Also, it looks like the fields 'freeTag' and 'pinnedMask' are never
initialized.
  Not sure, if it is intentional.

Otherwise, the fix looks good to me.

Thanks,
Serguei


On 9/28/17 18:20, Yasumasa Suenaga wrote:

Hi Serguei,

I added it to JBS:

https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248

Sorry for my English. I'm not good at English...


Please, don't worry about your English.
Your description looks good.
Thank you for the bug report update!

Thanks,
Serguei


Yasumasa



2017-09-29 8:27 GMT+09:00 [hidden email]
[hidden email]:

Hi Yasumasa,

Could you, please, also add some evaluation to the bug report about what is
the root cause and how do you fix it?

Thanks,
Serguei



On 9/26/17 18:10, Yasumasa Suenaga wrote:

Hi all,

I added noreg-hard label to JBS because this issue appears Stack
Memory window on HSDB (GUI application). So it is hard to test.

   https://bugs.openjdk.java.net/browse/JDK-8187403


Thanks,

Yasumasa



2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga [hidden email]:

Hi all,

I uploaded new webrev to be adapted to jdk10/hs:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/


Thanks,

Yasumasa


On 2017/09/21 7:48, Yasumasa Suenaga wrote:

PING:

Have you checked this issue?

    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/


Yasumasa



On 2017/09/11 11:18, Yasumasa Suenaga wrote:

Hi all,

This review request is a part of [1].


JBS:
    https://bugs.openjdk.java.net/browse/JDK-8187403

webrev:
    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/


I cannot access JPRT. So I need a sponsor.


Thanks,

Yasumasa


[1]

http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html



Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Yasumasa Suenaga-4
Thanks Serguei,

I've uploaded new webrev. Could you review again?
  http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.02/


Yasumasa



2017-09-29 16:49 GMT+09:00 [hidden email]
<[hidden email]>:

> Hi Yasumasa,
>
>
> On 9/28/17 23:21, Yasumasa Suenaga wrote:
>
> Hi Serguet,
>
> Thank you for your comment.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html
>
>   It seems, there is no reason for renaming 'type' to 't' in the
> initialize() method.
>
> I added new private member "type" as HeapRegionType.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.udiff.html
>
> So I renamed to "t" to avoid conflict.
>
>
> There is no conflict.
> Only local is used in the initialize() method.
> Also, the initialize() method is static so that the instance field 'type' is
> not in its scope.
> Otherwise, you could use this.type to avoid such a conflict.
>
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html
>
>   89     public HeapRegion addrToRegion(Address addr) {
>   90       return regions().getByAddress(addr);
>   91     }
>
>   A suggestion: replace 'addrToRegion' with 'getByAddress'.
>   It will look similar to the 'heapRegionIterator.'
>
> I've implemented it to follow HotSpot implementation.
>
> http://hg.openjdk.java.net/jdk10/hs/file/3a45532a1854/src/hotspot/share/gc/g1/heapRegionManager.inline.hpp#l32
>
> I think current proposal is easy to understand if other people check
> this with HotSpot.
> Should I rename to "getByAddress" ?
>
>
>   81     public Iterator<HeapRegion> heapRegionIterator() {
>   82         return regions().heapRegionIterator(length());
>   83     }
>  . . .
>   89     public HeapRegion addrToRegion(Address addr) {
>   90       return regions().getByAddress(addr);
>   91     }
>
>
>   There is already regions().getByAddress(addr), so I'm suggesting to follow
> this local pattern.
>   Renaming 'addrToRegion' to 'getByAddress' will unify it with the
> 'heapRegionIterator()'.
>   Otherwise, you would need to rename 'getByAddress' to 'addrToRegion'
> everywhere.
>   But I guess, it is better to avoid.
>
> Thanks,
> Serguei
>
>
>
> Other your comment will be fixed in new webrev later.
>
> Thanks,
>
> Yasumasa
>
>
> 2017-09-29 14:35 GMT+09:00 [hidden email]
> <[hidden email]>:
>
> Hi Yasumasa,
>
> Just some minor comments.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java.frames.html
>
> I'd suggest to make the lines 144-145 a one-liner.
> It won't be that big. Otherwise, the indent is not right.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html
>
>   The same as above for lines 85-86.
>   It seems, there is no reason for renaming 'type' to 't' in the
> initialize() method.
>
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html
>
>   89     public HeapRegion addrToRegion(Address addr) {
>   90       return regions().getByAddress(addr);
>   91     }
>
>   A suggestion: replace 'addrToRegion' with 'getByAddress'.
>   It will look similar to the 'heapRegionIterator.'
>
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html
>
>   41     private static int freeTag;
>   42
>   43     private static int youngMask;
>   44
>   45     private static int humongousMask;
>   46
>   47     private static int pinnedMask;
>   48
>   49     private static int oldMask;
>   50
>   51     private static CIntegerField tagField;
>
>   Unneeded empty lines.
>
>   Also, it looks like the fields 'freeTag' and 'pinnedMask' are never
> initialized.
>   Not sure, if it is intentional.
>
> Otherwise, the fix looks good to me.
>
> Thanks,
> Serguei
>
>
> On 9/28/17 18:20, Yasumasa Suenaga wrote:
>
> Hi Serguei,
>
> I added it to JBS:
>
> https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248
>
> Sorry for my English. I'm not good at English...
>
>
> Please, don't worry about your English.
> Your description looks good.
> Thank you for the bug report update!
>
> Thanks,
> Serguei
>
>
> Yasumasa
>
>
>
> 2017-09-29 8:27 GMT+09:00 [hidden email]
> <[hidden email]>:
>
> Hi Yasumasa,
>
> Could you, please, also add some evaluation to the bug report about what is
> the root cause and how do you fix it?
>
> Thanks,
> Serguei
>
>
>
> On 9/26/17 18:10, Yasumasa Suenaga wrote:
>
> Hi all,
>
> I added noreg-hard label to JBS because this issue appears Stack
> Memory window on HSDB (GUI application). So it is hard to test.
>
>    https://bugs.openjdk.java.net/browse/JDK-8187403
>
>
> Thanks,
>
> Yasumasa
>
>
>
> 2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <[hidden email]>:
>
> Hi all,
>
> I uploaded new webrev to be adapted to jdk10/hs:
>
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2017/09/21 7:48, Yasumasa Suenaga wrote:
>
> PING:
>
> Have you checked this issue?
>
>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>
>
> Yasumasa
>
>
>
> On 2017/09/11 11:18, Yasumasa Suenaga wrote:
>
> Hi all,
>
> This review request is a part of [1].
>
>
> JBS:
>     https://bugs.openjdk.java.net/browse/JDK-8187403
>
> webrev:
>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>
>
> I cannot access JPRT. So I need a sponsor.
>
>
> Thanks,
>
> Yasumasa
>
>
> [1]
>
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

serguei.spitsyn@oracle.com


On 9/29/17 01:25, Yasumasa Suenaga wrote:
> Thanks Serguei,
>
> I've uploaded new webrev. Could you review again?
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.02/

Looks good.
I can sponsor it, but you probably need another review.

Thanks,
Serguei


> Yasumasa
>
>
>
> 2017-09-29 16:49 GMT+09:00 [hidden email]
> <[hidden email]>:
>> Hi Yasumasa,
>>
>>
>> On 9/28/17 23:21, Yasumasa Suenaga wrote:
>>
>> Hi Serguet,
>>
>> Thank you for your comment.
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html
>>
>>    It seems, there is no reason for renaming 'type' to 't' in the
>> initialize() method.
>>
>> I added new private member "type" as HeapRegionType.
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.udiff.html
>>
>> So I renamed to "t" to avoid conflict.
>>
>>
>> There is no conflict.
>> Only local is used in the initialize() method.
>> Also, the initialize() method is static so that the instance field 'type' is
>> not in its scope.
>> Otherwise, you could use this.type to avoid such a conflict.
>>
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html
>>
>>    89     public HeapRegion addrToRegion(Address addr) {
>>    90       return regions().getByAddress(addr);
>>    91     }
>>
>>    A suggestion: replace 'addrToRegion' with 'getByAddress'.
>>    It will look similar to the 'heapRegionIterator.'
>>
>> I've implemented it to follow HotSpot implementation.
>>
>> http://hg.openjdk.java.net/jdk10/hs/file/3a45532a1854/src/hotspot/share/gc/g1/heapRegionManager.inline.hpp#l32
>>
>> I think current proposal is easy to understand if other people check
>> this with HotSpot.
>> Should I rename to "getByAddress" ?
>>
>>
>>    81     public Iterator<HeapRegion> heapRegionIterator() {
>>    82         return regions().heapRegionIterator(length());
>>    83     }
>>   . . .
>>    89     public HeapRegion addrToRegion(Address addr) {
>>    90       return regions().getByAddress(addr);
>>    91     }
>>
>>
>>    There is already regions().getByAddress(addr), so I'm suggesting to follow
>> this local pattern.
>>    Renaming 'addrToRegion' to 'getByAddress' will unify it with the
>> 'heapRegionIterator()'.
>>    Otherwise, you would need to rename 'getByAddress' to 'addrToRegion'
>> everywhere.
>>    But I guess, it is better to avoid.
>>
>> Thanks,
>> Serguei
>>
>>
>>
>> Other your comment will be fixed in new webrev later.
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> 2017-09-29 14:35 GMT+09:00 [hidden email]
>> <[hidden email]>:
>>
>> Hi Yasumasa,
>>
>> Just some minor comments.
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java.frames.html
>>
>> I'd suggest to make the lines 144-145 a one-liner.
>> It won't be that big. Otherwise, the indent is not right.
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html
>>
>>    The same as above for lines 85-86.
>>    It seems, there is no reason for renaming 'type' to 't' in the
>> initialize() method.
>>
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html
>>
>>    89     public HeapRegion addrToRegion(Address addr) {
>>    90       return regions().getByAddress(addr);
>>    91     }
>>
>>    A suggestion: replace 'addrToRegion' with 'getByAddress'.
>>    It will look similar to the 'heapRegionIterator.'
>>
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html
>>
>>    41     private static int freeTag;
>>    42
>>    43     private static int youngMask;
>>    44
>>    45     private static int humongousMask;
>>    46
>>    47     private static int pinnedMask;
>>    48
>>    49     private static int oldMask;
>>    50
>>    51     private static CIntegerField tagField;
>>
>>    Unneeded empty lines.
>>
>>    Also, it looks like the fields 'freeTag' and 'pinnedMask' are never
>> initialized.
>>    Not sure, if it is intentional.
>>
>> Otherwise, the fix looks good to me.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 9/28/17 18:20, Yasumasa Suenaga wrote:
>>
>> Hi Serguei,
>>
>> I added it to JBS:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248
>>
>> Sorry for my English. I'm not good at English...
>>
>>
>> Please, don't worry about your English.
>> Your description looks good.
>> Thank you for the bug report update!
>>
>> Thanks,
>> Serguei
>>
>>
>> Yasumasa
>>
>>
>>
>> 2017-09-29 8:27 GMT+09:00 [hidden email]
>> <[hidden email]>:
>>
>> Hi Yasumasa,
>>
>> Could you, please, also add some evaluation to the bug report about what is
>> the root cause and how do you fix it?
>>
>> Thanks,
>> Serguei
>>
>>
>>
>> On 9/26/17 18:10, Yasumasa Suenaga wrote:
>>
>> Hi all,
>>
>> I added noreg-hard label to JBS because this issue appears Stack
>> Memory window on HSDB (GUI application). So it is hard to test.
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8187403
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>
>> 2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <[hidden email]>:
>>
>> Hi all,
>>
>> I uploaded new webrev to be adapted to jdk10/hs:
>>
>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2017/09/21 7:48, Yasumasa Suenaga wrote:
>>
>> PING:
>>
>> Have you checked this issue?
>>
>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>
>>
>> Yasumasa
>>
>>
>>
>> On 2017/09/11 11:18, Yasumasa Suenaga wrote:
>>
>> Hi all,
>>
>> This review request is a part of [1].
>>
>>
>> JBS:
>>      https://bugs.openjdk.java.net/browse/JDK-8187403
>>
>> webrev:
>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>>
>>
>> I cannot access JPRT. So I need a sponsor.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> [1]
>>
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>>
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Yasumasa Suenaga-4
Thanks Serguei,

I'm waiting another reviewer.


Yasumasa


2017/09/29 午後6:00 "[hidden email]" <[hidden email]>:


On 9/29/17 01:25, Yasumasa Suenaga wrote:
Thanks Serguei,

I've uploaded new webrev. Could you review again?
   http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.02/

Looks good.
I can sponsor it, but you probably need another review.

Thanks,
Serguei


Yasumasa



2017-09-29 16:49 GMT+09:00 [hidden email]
<[hidden email]>:
Hi Yasumasa,


On 9/28/17 23:21, Yasumasa Suenaga wrote:

Hi Serguet,

Thank you for your comment.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html

   It seems, there is no reason for renaming 'type' to 't' in the
initialize() method.

I added new private member "type" as HeapRegionType.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.udiff.html

So I renamed to "t" to avoid conflict.


There is no conflict.
Only local is used in the initialize() method.
Also, the initialize() method is static so that the instance field 'type' is
not in its scope.
Otherwise, you could use this.type to avoid such a conflict.


http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html

   89     public HeapRegion addrToRegion(Address addr) {
   90       return regions().getByAddress(addr);
   91     }

   A suggestion: replace 'addrToRegion' with 'getByAddress'.
   It will look similar to the 'heapRegionIterator.'

I've implemented it to follow HotSpot implementation.

http://hg.openjdk.java.net/jdk10/hs/file/3a45532a1854/src/hotspot/share/gc/g1/heapRegionManager.inline.hpp#l32

I think current proposal is easy to understand if other people check
this with HotSpot.
Should I rename to "getByAddress" ?


   81     public Iterator<HeapRegion> heapRegionIterator() {
   82         return regions().heapRegionIterator(length());
   83     }
  . . .
   89     public HeapRegion addrToRegion(Address addr) {
   90       return regions().getByAddress(addr);
   91     }


   There is already regions().getByAddress(addr), so I'm suggesting to follow
this local pattern.
   Renaming 'addrToRegion' to 'getByAddress' will unify it with the
'heapRegionIterator()'.
   Otherwise, you would need to rename 'getByAddress' to 'addrToRegion'
everywhere.
   But I guess, it is better to avoid.

Thanks,
Serguei



Other your comment will be fixed in new webrev later.

Thanks,

Yasumasa


2017-09-29 14:35 GMT+09:00 [hidden email]
<[hidden email]>:

Hi Yasumasa,

Just some minor comments.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java.frames.html

I'd suggest to make the lines 144-145 a one-liner.
It won't be that big. Otherwise, the indent is not right.

http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html

   The same as above for lines 85-86.
   It seems, there is no reason for renaming 'type' to 't' in the
initialize() method.


http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html

   89     public HeapRegion addrToRegion(Address addr) {
   90       return regions().getByAddress(addr);
   91     }

   A suggestion: replace 'addrToRegion' with 'getByAddress'.
   It will look similar to the 'heapRegionIterator.'


http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html

   41     private static int freeTag;
   42
   43     private static int youngMask;
   44
   45     private static int humongousMask;
   46
   47     private static int pinnedMask;
   48
   49     private static int oldMask;
   50
   51     private static CIntegerField tagField;

   Unneeded empty lines.

   Also, it looks like the fields 'freeTag' and 'pinnedMask' are never
initialized.
   Not sure, if it is intentional.

Otherwise, the fix looks good to me.

Thanks,
Serguei


On 9/28/17 18:20, Yasumasa Suenaga wrote:

Hi Serguei,

I added it to JBS:

https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248

Sorry for my English. I'm not good at English...


Please, don't worry about your English.
Your description looks good.
Thank you for the bug report update!

Thanks,
Serguei


Yasumasa



2017-09-29 8:27 GMT+09:00 [hidden email]
<[hidden email]>:

Hi Yasumasa,

Could you, please, also add some evaluation to the bug report about what is
the root cause and how do you fix it?

Thanks,
Serguei



On 9/26/17 18:10, Yasumasa Suenaga wrote:

Hi all,

I added noreg-hard label to JBS because this issue appears Stack
Memory window on HSDB (GUI application). So it is hard to test.

    https://bugs.openjdk.java.net/browse/JDK-8187403


Thanks,

Yasumasa



2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <[hidden email]>:

Hi all,

I uploaded new webrev to be adapted to jdk10/hs:

    http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/


Thanks,

Yasumasa


On 2017/09/21 7:48, Yasumasa Suenaga wrote:

PING:

Have you checked this issue?

     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/


Yasumasa



On 2017/09/11 11:18, Yasumasa Suenaga wrote:

Hi all,

This review request is a part of [1].


JBS:
     https://bugs.openjdk.java.net/browse/JDK-8187403

webrev:
     http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/


I cannot access JPRT. So I need a sponsor.


Thanks,

Yasumasa


[1]

http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html




Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Jini George
Your changes look good, Yasumasa.

Thanks,
Jini (Not a Reviewer).

On 9/29/2017 2:49 PM, Yasumasa Suenaga wrote:

> Thanks Serguei,
>
> I'm waiting another reviewer.
>
>
> Yasumasa
>
>
> 2017/09/29 午後6:00 "[hidden email]
> <mailto:[hidden email]>" <[hidden email]
> <mailto:[hidden email]>>:
>
>
>
>     On 9/29/17 01:25, Yasumasa Suenaga wrote:
>
>         Thanks Serguei,
>
>         I've uploaded new webrev. Could you review again?
>         http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.02/
>         <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.02/>
>
>
>     Looks good.
>     I can sponsor it, but you probably need another review.
>
>     Thanks,
>     Serguei
>
>
>         Yasumasa
>
>
>
>         2017-09-29 16:49 GMT+09:00 [hidden email]
>         <mailto:[hidden email]>
>         <[hidden email] <mailto:[hidden email]>>:
>
>             Hi Yasumasa,
>
>
>             On 9/28/17 23:21, Yasumasa Suenaga wrote:
>
>             Hi Serguet,
>
>             Thank you for your comment.
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html>
>
>                 It seems, there is no reason for renaming 'type' to 't'
>             in the
>             initialize() method.
>
>             I added new private member "type" as HeapRegionType.
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.udiff.html
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.udiff.html>
>
>             So I renamed to "t" to avoid conflict.
>
>
>             There is no conflict.
>             Only local is used in the initialize() method.
>             Also, the initialize() method is static so that the instance
>             field 'type' is
>             not in its scope.
>             Otherwise, you could use this.type to avoid such a conflict.
>
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html>
>
>                 89     public HeapRegion addrToRegion(Address addr) {
>                 90       return regions().getByAddress(addr);
>                 91     }
>
>                 A suggestion: replace 'addrToRegion' with 'getByAddress'.
>                 It will look similar to the 'heapRegionIterator.'
>
>             I've implemented it to follow HotSpot implementation.
>
>             http://hg.openjdk.java.net/jdk10/hs/file/3a45532a1854/src/hotspot/share/gc/g1/heapRegionManager.inline.hpp#l32
>             <http://hg.openjdk.java.net/jdk10/hs/file/3a45532a1854/src/hotspot/share/gc/g1/heapRegionManager.inline.hpp#l32>
>
>             I think current proposal is easy to understand if other
>             people check
>             this with HotSpot.
>             Should I rename to "getByAddress" ?
>
>
>                 81     public Iterator<HeapRegion> heapRegionIterator() {
>                 82         return regions().heapRegionIterator(length());
>                 83     }
>                . . .
>                 89     public HeapRegion addrToRegion(Address addr) {
>                 90       return regions().getByAddress(addr);
>                 91     }
>
>
>                 There is already regions().getByAddress(addr), so I'm
>             suggesting to follow
>             this local pattern.
>                 Renaming 'addrToRegion' to 'getByAddress' will unify it
>             with the
>             'heapRegionIterator()'.
>                 Otherwise, you would need to rename 'getByAddress' to
>             'addrToRegion'
>             everywhere.
>                 But I guess, it is better to avoid.
>
>             Thanks,
>             Serguei
>
>
>
>             Other your comment will be fixed in new webrev later.
>
>             Thanks,
>
>             Yasumasa
>
>
>             2017-09-29 14:35 GMT+09:00 [hidden email]
>             <mailto:[hidden email]>
>             <[hidden email]
>             <mailto:[hidden email]>>:
>
>             Hi Yasumasa,
>
>             Just some minor comments.
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java.frames.html
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java.frames.html>
>
>             I'd suggest to make the lines 144-145 a one-liner.
>             It won't be that big. Otherwise, the indent is not right.
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegion.java.frames.html>
>
>                 The same as above for lines 85-86.
>                 It seems, there is no reason for renaming 'type' to 't'
>             in the
>             initialize() method.
>
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionManager.java.frames.html>
>
>                 89     public HeapRegion addrToRegion(Address addr) {
>                 90       return regions().getByAddress(addr);
>                 91     }
>
>                 A suggestion: replace 'addrToRegion' with 'getByAddress'.
>                 It will look similar to the 'heapRegionIterator.'
>
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html>
>
>                 41     private static int freeTag;
>                 42
>                 43     private static int youngMask;
>                 44
>                 45     private static int humongousMask;
>                 46
>                 47     private static int pinnedMask;
>                 48
>                 49     private static int oldMask;
>                 50
>                 51     private static CIntegerField tagField;
>
>                 Unneeded empty lines.
>
>                 Also, it looks like the fields 'freeTag' and
>             'pinnedMask' are never
>             initialized.
>                 Not sure, if it is intentional.
>
>             Otherwise, the fix looks good to me.
>
>             Thanks,
>             Serguei
>
>
>             On 9/28/17 18:20, Yasumasa Suenaga wrote:
>
>             Hi Serguei,
>
>             I added it to JBS:
>
>             https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248
>             <https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248>
>
>             Sorry for my English. I'm not good at English...
>
>
>             Please, don't worry about your English.
>             Your description looks good.
>             Thank you for the bug report update!
>
>             Thanks,
>             Serguei
>
>
>             Yasumasa
>
>
>
>             2017-09-29 8:27 GMT+09:00 [hidden email]
>             <mailto:[hidden email]>
>             <[hidden email]
>             <mailto:[hidden email]>>:
>
>             Hi Yasumasa,
>
>             Could you, please, also add some evaluation to the bug
>             report about what is
>             the root cause and how do you fix it?
>
>             Thanks,
>             Serguei
>
>
>
>             On 9/26/17 18:10, Yasumasa Suenaga wrote:
>
>             Hi all,
>
>             I added noreg-hard label to JBS because this issue appears Stack
>             Memory window on HSDB (GUI application). So it is hard to test.
>
>             https://bugs.openjdk.java.net/browse/JDK-8187403
>             <https://bugs.openjdk.java.net/browse/JDK-8187403>
>
>
>             Thanks,
>
>             Yasumasa
>
>
>
>             2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga
>             <[hidden email] <mailto:[hidden email]>>:
>
>             Hi all,
>
>             I uploaded new webrev to be adapted to jdk10/hs:
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/>
>
>
>             Thanks,
>
>             Yasumasa
>
>
>             On 2017/09/21 7:48, Yasumasa Suenaga wrote:
>
>             PING:
>
>             Have you checked this issue?
>
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/>
>
>
>             Yasumasa
>
>
>
>             On 2017/09/11 11:18, Yasumasa Suenaga wrote:
>
>             Hi all,
>
>             This review request is a part of [1].
>
>
>             JBS:
>             https://bugs.openjdk.java.net/browse/JDK-8187403
>             <https://bugs.openjdk.java.net/browse/JDK-8187403>
>
>             webrev:
>             http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/
>             <http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/>
>
>
>             I cannot access JPRT. So I need a sponsor.
>
>
>             Thanks,
>
>             Yasumasa
>
>
>             [1]
>
>             http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html
>             <http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html>
>
>
>
>