Thanks Serguei, I'm waiting another reviewer.
Yasumasa 2017/09/29 午後6:00 "serguei.spit...@oracle.com" <serguei.spit...@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 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?focusedComm >>> entId=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/20 >>> 17-September/021821.html >>> >>> >>> >>> >
