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