Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Jini George Fri, 06 Oct 2017 08:29:30 -0700

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.spit...@oracle.com<mailto:serguei.spit...@oracle.com>" <serguei.spit...@oracle.com<mailto: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/
        <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
        <mailto:serguei.spit...@oracle.com>
        <serguei.spit...@oracle.com <mailto: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
            
<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.spit...@oracle.com
            <mailto:serguei.spit...@oracle.com>
            <serguei.spit...@oracle.com
            <mailto: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
            
<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.spit...@oracle.com
            <mailto:serguei.spit...@oracle.com>
            <serguei.spit...@oracle.com
            <mailto: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
            <https://bugs.openjdk.java.net/browse/JDK-8187403>


            Thanks,

            Yasumasa



            2017-09-26 23:55 GMT+09:00 Yasumasa Suenaga
            <yasue...@gmail.com <mailto: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/
            <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>

Re: RFR: 8187403: [Unknown generation] is shown in Stack Memory on HSDB

Reply via email to