RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB
Jini George
jini.george at oracle.com
Fri Oct 6 15:28:25 UTC 2017
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 "serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com>" <serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at 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/
> <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.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com>
> <serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at 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
> <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 serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com>
> <serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at 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
> <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 serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com>
> <serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at 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
> <https://bugs.openjdk.java.net/browse/JDK-8187403>
>
>
> Thanks,
>
> Yasumasa
>
>
>
> 2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga
> <yasuenag at gmail.com <mailto:yasuenag at gmail.com>>:
>
> 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>
>
>
>
>
More information about the serviceability-dev
mailing list