RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Sep 29 09:00:12 UTC 2017
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.spitsyn at oracle.com
> <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
>>
>> 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.spitsyn at oracle.com
>> <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
>>
>> 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.spitsyn at oracle.com
>> <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
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>
>> 2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <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/
>>
>>
>> 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
>>
>>
>>
More information about the serviceability-dev
mailing list