RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB
Yasumasa Suenaga
yasuenag at gmail.com
Fri Sep 29 09:19:25 UTC 2017
Thanks Serguei,
I'm waiting another reviewer.
Yasumasa
2017/09/29 午後6:00 "serguei.spitsyn at oracle.com" <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/
>>
>
> 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?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.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/20
>>> 17-September/021821.html
>>>
>>>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170929/9056f1e5/attachment-0001.html>
More information about the serviceability-dev
mailing list