RFR: 8190837: BasicType and BasicTypeSize should refer to HotSpot values
Jini George
jini.george at oracle.com
Thu Nov 30 18:04:57 UTC 2017
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 at oracle.com
> <mailto:david.holmes at 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 at oracle.com
> <mailto:david.holmes at 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 at oracle.com
> <mailto:chris.plummer at 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 at oracle.com
> <mailto:chris.plummer at 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>
>
>
>
>
More information about the serviceability-dev
mailing list