[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB
From: Yasumasa Suenaga <yasuenag () gmail ! com>
Date: 2017-09-29 9:19:25
Message-ID: CAGFVN2C=uiSLBn=CtokPu+4nP3ZHAWibq2RSY96nN_uywsHGHA () mail ! gmail ! com
[Download RAW message or body]
Thanks Serguei,
I'm waiting another reviewer.
Yasumasa
2017/09/29 午後6:00 "serguei.spitsyn@oracle.com" <serguei.spitsyn@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@oracle.com
>> <serguei.spitsyn@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@oracle.com
>>> <serguei.spitsyn@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@oracle.com
>>> <serguei.spitsyn@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@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
>>>
>>>
>>>
>>>
>
[Attachment #3 (text/html)]
<div dir="auto">Thanks Serguei,<div dir="auto"><br></div><div dir="auto">I'm \
waiting another reviewer.</div><div dir="auto"><br></div><div \
dir="auto"><br></div><div dir="auto">Yasumasa</div><div \
dir="auto"><br></div></div><div class="gmail_extra"><br><div \
class="gmail_quote">2017/09/29 午後6:00 "<a \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>" <<a \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>>:<br \
type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><br> <br>
On 9/29/17 01:25, Yasumasa Suenaga wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> Thanks Serguei,<br>
<br>
I've uploaded new webrev. Could you review again?<br>
<a href="http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.02/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~y<wbr>suenaga/JDK-8187403/webrev.02/</a><br>
</blockquote>
<br>
Looks good.<br>
I can sponsor it, but you probably need another review.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> Yasumasa<br>
<br>
<br>
<br>
2017-09-29 16:49 GMT+09:00 <a href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a><br> <<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>>:<br> <blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> Hi Yasumasa,<br>
<br>
<br>
On 9/28/17 23:21, Yasumasa Suenaga wrote:<br>
<br>
Hi Serguet,<br>
<br>
Thank you for your comment.<br>
<br>
<a href="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" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ys<wbr>uenaga/JDK-8187403 \
/webrev.01/<wbr>src/jdk.hotspot.agent/share/<wbr>classes/sun/jvm/hotspot/gc/g1/<wbr>HeapRegion.java.frames.html</a><br>
<br>
It seems, there is no reason for renaming 'type' to 't' in \
the<br> initialize() method.<br>
<br>
I added new private member "type" as HeapRegionType.<br>
<br>
<a href="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" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ys<wbr>uenaga/JDK-8187403 \
/webrev.01/<wbr>src/jdk.hotspot.agent/share/<wbr>classes/sun/jvm/hotspot/gc/g1/<wbr>HeapRegion.java.udiff.html</a><br>
<br>
So I renamed to "t" to avoid conflict.<br>
<br>
<br>
There is no conflict.<br>
Only local is used in the initialize() method.<br>
Also, the initialize() method is static so that the instance field 'type' \
is<br> not in its scope.<br>
Otherwise, you could use this.type to avoid such a conflict.<br>
<br>
<br>
<a href="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" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ys<wbr>uenaga/JDK-8187403 \
/webrev.01/<wbr>src/jdk.hotspot.agent/share/<wbr>classes/sun/jvm/hotspot/gc/g1/<wbr>HeapRegionManager.java.frames.<wbr>html</a><br>
<br>
89 public HeapRegion addrToRegion(Address addr) {<br>
90 return regions().getByAddress(addr);<br>
91 }<br>
<br>
A suggestion: replace 'addrToRegion' with 'getByAddress'.<br>
It will look similar to the 'heapRegionIterator.'<br>
<br>
I've implemented it to follow HotSpot implementation.<br>
<br>
<a href="http://hg.openjdk.java.net/jdk10/hs/file/3a45532a1854/src/hotspot/share/gc/g1/heapRegionManager.inline.hpp#l32" \
rel="noreferrer" target="_blank">http://hg.openjdk.java.net/jdk<wbr>10/hs/file/3a45532a1854/src/<wbr>hotspot/share/gc/g1/heapRegion<wbr>Manager.inline.hpp#l32</a><br>
<br>
I think current proposal is easy to understand if other people check<br>
this with HotSpot.<br>
Should I rename to "getByAddress" ?<br>
<br>
<br>
81 public Iterator<HeapRegion> heapRegionIterator() {<br>
82 return regions().heapRegionIterator(l<wbr>ength());<br>
83 }<br>
. . .<br>
89 public HeapRegion addrToRegion(Address addr) {<br>
90 return regions().getByAddress(addr);<br>
91 }<br>
<br>
<br>
There is already regions().getByAddress(addr), so I'm suggesting to \
follow<br> this local pattern.<br>
Renaming 'addrToRegion' to 'getByAddress' will unify it with \
the<br> 'heapRegionIterator()'.<br>
Otherwise, you would need to rename 'getByAddress' to \
'addrToRegion'<br> everywhere.<br>
But I guess, it is better to avoid.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
Other your comment will be fixed in new webrev later.<br>
<br>
Thanks,<br>
<br>
Yasumasa<br>
<br>
<br>
2017-09-29 14:35 GMT+09:00 <a href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a><br> <<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>>:<br> <br>
Hi Yasumasa,<br>
<br>
Just some minor comments.<br>
<br>
<a href="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" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ys<wbr>uenaga/JDK-8187403 \
/webrev.01/<wbr>src/jdk.hotspot.agent/share/<wbr>classes/sun/jvm/hotspot/gc/g1/<wbr>G1HeapRegionTable.java.frames.<wbr>html</a><br>
<br>
I'd suggest to make the lines 144-145 a one-liner.<br>
It won't be that big. Otherwise, the indent is not right.<br>
<br>
<a href="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" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ys<wbr>uenaga/JDK-8187403 \
/webrev.01/<wbr>src/jdk.hotspot.agent/share/<wbr>classes/sun/jvm/hotspot/gc/g1/<wbr>HeapRegion.java.frames.html</a><br>
<br>
The same as above for lines 85-86.<br>
It seems, there is no reason for renaming 'type' to 't' in \
the<br> initialize() method.<br>
<br>
<br>
<a href="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" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ys<wbr>uenaga/JDK-8187403 \
/webrev.01/<wbr>src/jdk.hotspot.agent/share/<wbr>classes/sun/jvm/hotspot/gc/g1/<wbr>HeapRegionManager.java.frames.<wbr>html</a><br>
<br>
89 public HeapRegion addrToRegion(Address addr) {<br>
90 return regions().getByAddress(addr);<br>
91 }<br>
<br>
A suggestion: replace 'addrToRegion' with 'getByAddress'.<br>
It will look similar to the 'heapRegionIterator.'<br>
<br>
<br>
<a href="http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/HeapRegionType.java.html" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ys<wbr>uenaga/JDK-8187403 \
/webrev.01/<wbr>src/jdk.hotspot.agent/share/<wbr>classes/sun/jvm/hotspot/gc/g1/<wbr>HeapRegionType.java.html</a><br>
<br>
41 private static int freeTag;<br>
42<br>
43 private static int youngMask;<br>
44<br>
45 private static int humongousMask;<br>
46<br>
47 private static int pinnedMask;<br>
48<br>
49 private static int oldMask;<br>
50<br>
51 private static CIntegerField tagField;<br>
<br>
Unneeded empty lines.<br>
<br>
Also, it looks like the fields 'freeTag' and 'pinnedMask' are \
never<br> initialized.<br>
Not sure, if it is intentional.<br>
<br>
Otherwise, the fix looks good to me.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 9/28/17 18:20, Yasumasa Suenaga wrote:<br>
<br>
Hi Serguei,<br>
<br>
I added it to JBS:<br>
<br>
<a href="https://bugs.openjdk.java.net/browse/JDK-8187403?focusedCommentId=14119248&am \
p;page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14119248" \
rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/<wbr>browse/JDK-8187403 \
?focusedComm<wbr>entId=14119248&page=com.<wbr>atlassian.jira.plugin.system.<wbr>issuetabpanels:comment-<wbr>tabpanel#comment-14119248</a><br>
<br>
Sorry for my English. I'm not good at English...<br>
<br>
<br>
Please, don't worry about your English.<br>
Your description looks good.<br>
Thank you for the bug report update!<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
Yasumasa<br>
<br>
<br>
<br>
2017-09-29 8:27 GMT+09:00 <a href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a><br> <<a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a>>:<br> <br>
Hi Yasumasa,<br>
<br>
Could you, please, also add some evaluation to the bug report about what is<br>
the root cause and how do you fix it?<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 9/26/17 18:10, Yasumasa Suenaga wrote:<br>
<br>
Hi all,<br>
<br>
I added noreg-hard label to JBS because this issue appears Stack<br>
Memory window on HSDB (GUI application). So it is hard to test.<br>
<br>
<a href="https://bugs.openjdk.java.net/browse/JDK-8187403" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/<wbr>browse/JDK-8187403</a><br> <br>
<br>
Thanks,<br>
<br>
Yasumasa<br>
<br>
<br>
<br>
2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga <<a href="mailto:yasuenag@gmail.com" \
target="_blank">yasuenag@gmail.com</a>>:<br> <br>
Hi all,<br>
<br>
I uploaded new webrev to be adapted to jdk10/hs:<br>
<br>
<a href="http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.01/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ys<wbr>uenaga/JDK-8187403/webrev.01/</a><br>
<br>
<br>
Thanks,<br>
<br>
Yasumasa<br>
<br>
<br>
On 2017/09/21 7:48, Yasumasa Suenaga wrote:<br>
<br>
PING:<br>
<br>
Have you checked this issue?<br>
<br>
<a href="http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~y<wbr>suenaga/JDK-8187403/webrev.00/</a><br>
<br>
<br>
Yasumasa<br>
<br>
<br>
<br>
On 2017/09/11 11:18, Yasumasa Suenaga wrote:<br>
<br>
Hi all,<br>
<br>
This review request is a part of [1].<br>
<br>
<br>
JBS:<br>
<a href="https://bugs.openjdk.java.net/browse/JDK-8187403" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.<wbr>net/browse/JDK-8187403</a><br> <br>
webrev:<br>
<a href="http://cr.openjdk.java.net/~ysuenaga/JDK-8187403/webrev.00/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~y<wbr>suenaga/JDK-8187403/webrev.00/</a><br>
<br>
<br>
I cannot access JPRT. So I need a sponsor.<br>
<br>
<br>
Thanks,<br>
<br>
Yasumasa<br>
<br>
<br>
[1]<br>
<br>
<a href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021821.html" \
rel="noreferrer" target="_blank">http://mail.openjdk.java.net/p<wbr>ipermail/serviceability-dev/20<wbr>17-September/021821.html</a><br>
<br>
<br>
<br>
</blockquote></blockquote>
<br>
</blockquote></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic