'Re: RFR: 8190837: BasicType and BasicTypeSize should refer to HotSpot values'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8190837: BasicType and BasicTypeSize should refer to HotSpot values
From:       Jini George <jini.george () oracle ! com>
Date:       2017-11-30 18:16:57
Message-ID: e49f4bb7-4449-1519-dbb3-8ba6f6d7685a () oracle ! com
[Download RAW message or body]

Thank you, Yasumasa, for doing this. Your changes look good to me.

Thanks,
Jini.

On 11/29/2017 3:34 PM, Yasumasa Suenaga wrote:
> Thanks David,
> 
> I will move setType() to after L250.
> And I'm waiting for second reviewer and sponsor.
> 
> 
> Yasumasa
> 
> 
> 2017/11/29 午後6:58 "David Holmes" <david.holmes@oracle.com 
> <mailto:david.holmes@oracle.com>>:
> 
> Hi Yasumasa,
> 
> On 29/11/2017 6:42 PM, Yasumasa Suenaga wrote:
> 
> Hi David,
> 
> That can be fixed using a no-arg
> constructor for static initialization and adding a private
> setType method
> used for the real initialization.
> 
> 
> I uploaded new webrev. Is it okay?
> 
> http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.01/
> <http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.01/>
> 
> 
> Minor nit: The private setType method should be defined after:
> 
> 250     //-- Internals only below this point
> 
> but otherwise this looks exactly like I had envisaged. No need to
> see updated webrev.
> 
> 
> I'm not at all clear why we need the tXxx and T_XXX forms?
> The former can be
> obtained from the latter.
> 
> 
> I agree with you, but it is difficult.
> For example, [1] has a lot of lines which use BasicType.
> 
> I had a lot of compile errors when I removed getTXxx methods
> from BasicType...
> 
> 
> I wasn't suggesting getting rid of the getTXxx methods just the tXxx
> fields - as you have done.
> 
> Thanks,
> David
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> [1]
> http://hg.openjdk.java.net/jdk/hs/file/5a449dbca6d0/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/Bytecodes.java#l560
>  <http://hg.openjdk.java.net/jdk/hs/file/5a449dbca6d0/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/Bytecodes.java#l560>
>  
> 
> 2017-11-29 16:01 GMT+09:00 David Holmes <david.holmes@oracle.com
> <mailto:david.holmes@oracle.com>>:
> 
> On 29/11/2017 4:19 PM, Jini George wrote:
> 
> 
> Hi Chris,
> 
> Thank you for raising this. I agree it is disruptive,
> but we are trying to
> address the issue of frequent SA breakages with hotspot
> changes, and the
> causes of these breakages. One of the reasons is the
> redefinition of
> constants, which is extremely error prone. There have
> been multiple cases
> where the changes get done in hotspot and the
> corresponding changes get
> inadvertently missed out in SA. And this does not get
> caught, sometimes, for
> months. I believe that the switch case statements
> conversion to if-else
> statements is not a heavy price to pay for avoiding
> errors like these.
> 
> 
> 
> I agree it is good to ensure this always matches the VM. I
> also agree it is
> unfortunate we lost the ability to keep the switch
> statements - so be it.
> I'm more concerned that the BasicType static fields are no
> longer final
> (that may raise parfait warnings!). That can be fixed using
> a no-arg
> constructor for static initialization and adding a private
> setType method
> used for the real initialization.
> 
> I'm not at all clear why we need the tXxx and T_XXX forms?
> The former can be
> obtained from the latter. And with the change to use the
> getTXxx() functions
> I think we can actually do away with all the tXxx static
> fields. The
> getTXxx() functions can be implemented as "return
> T_XXX.getType(); and the
> intToBasicType() function can also examine getType(). The
> name could be
> stored as a field, set at construction time. Just a thought. :)
> 
> Thanks,
> David
> 
> 
> Thanks!
> - Jini.
> 
> On 11/29/2017 7:56 AM, Chris Plummer wrote:
> 
> 
> On 11/28/17 5:23 PM, Yasumasa Suenaga wrote:
> 
> 
> Hi Chris,
> 
> I understood the reason for getting rid of
> the case statements. I was
> just
> wondering if you weighed this code
> disruption vs. the value of what you
> are
> fixing.
> 
> 
> Jini has pointed it as below and I agree with him:
> 
> 
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
> <http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html>
>                 
> -------------
> One point I want to make is that we have the
> enum BasicTypeSize redefined in SA as public
> static final values, and
> this makes it error prone when existing enum
> values change, just as in
> this case. An ideal solution would be to include
> this in vmStructs.cpp
> as a declare_constant() macro, and read this in
> SA with the
> db.lookupIntConstant() method.
> -------------
> 
> 
> Hi Yasumasa,
> 
> Yes, I had read that and understand the point being
> made. What I'm asking
> is now that you've implemented it and seen the
> disruption to the switch
> statements (which I assume you and Jini were not
> aware of before embarking
> on this), is it still worth doing? It's not really
> that big of a deal to me.
> I just want to make sure it's been taken into
> consideration.
> 
> thanks,
> 
> Chris
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> 2017-11-29 10:09 GMT+09:00 Chris Plummer
> <chris.plummer@oracle.com
> <mailto:chris.plummer@oracle.com>>:
> 
> 
> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
> 
> 
> Hi Chris,
> 
> 2017-11-29 5:32 GMT+09:00 Chris Plummer
> <chris.plummer@oracle.com
> <mailto:chris.plummer@oracle.com>>:
> 
> 
> Hi Yasumasa,
> 
> This isn't code I know very well,
> and I'm not a Reviewer. Just a
> couple
> of
> observations.
> 
> I wonder if the person who
> originally suggested this change
> realized
> the
> disruption it would have to existing
> switch statements. I'm not
> saying
> the
> change shouldn't be done for this
> reason, but it is something to at
> least
> consider.
> 
> 
> According to JLS, `case` label need to
> have constant expression.
> We cannot set `static final` to the
> field which is set at
> initialize().
> 
> 
> https://docs.oracle.com/javase/specs/jls/se9/html/jls-14.html#jls-14.11
> <https://docs.oracle.com/javase/specs/jls/se9/html/jls-14.html#jls-14.11>
> 
> 
> I understood the reason for getting rid of
> the case statements. I was
> just
> wondering if you weighed this code
> disruption vs. the value of what you
> are
> fixing.
> 
> 
> 
> 
> Your coding pattern for the
> following differs from the existing 200+
> instances of
> VM.registerVMInitializedObserver()
> calls already in
> place. I
> suggest you be consistent with
> existing code.
> 
> 71     static {
> 72   
> VM.registerVMInitializedObserver(
> 73              (o, d) ->
> initialize(VM.getVM().getTypeDataBase()));
> 74     }
> 
> 
> This style has been used in
> JavaThreadsPanel.java .
> 
> 
> Ah, I missed that one case, but then it's
> one that you added. :)
> 
> 
> I like it because it is simple.
> 
> I will change it to traditional style if
> other reviewer(s) suggest it.
> 
> 
> I think consistency is important.
> 
> thanks,
> 
> Chris
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> thanks,
> 
> Chris
> 
> 
> On 11/27/17 11:49 PM, Yasumasa
> Suenaga wrote:
> 
> Hi all,
> 
> Enum values in BasicType and
> BasicTypeSize are declared as const
> values. However, it makes error
> prone when existing enum values
> change.
> They should refer to HotSpot values
> via VMStructs.
> 
> This issue has been pointed by Jini [1].
> 
> I uploaded webrev for this issue.
> Could you review it?
> 
> http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.00/
> <http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.00/>
> 
> I cannot access JPRT. So I need a
> sponsor.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> [1]
> 
> 
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
> <http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html>
>  
> 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic