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

serguei.spit...@oracle.com Fri, 29 Sep 2017 02:01:23 -0700


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 serguei.spit...@oracle.com
<serguei.spit...@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 serguei.spit...@oracle.com
<serguei.spit...@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 serguei.spit...@oracle.com
<serguei.spit...@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 <yasue...@gmail.com>:

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

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

Reply via email to