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