RFR: 8190837: BasicType and BasicTypeSize should refer to HotSpot values
Yasumasa Suenaga
yasuenag at gmail.com
Fri Dec 1 01:15:13 UTC 2017
Hi David,
I've tested this change with test/hotspot/jtreg/serviceability/sa on Linux x64.
Thanks,
Yasumasa
2017-12-01 10:11 GMT+09:00 David Holmes <david.holmes at oracle.com>:
> On 29/11/2017 8:04 PM, Yasumasa Suenaga wrote:
>>
>> Thanks David,
>>
>> I will move setType() to after L250.
>> And I'm waiting for second reviewer and sponsor.
>
>
> I can sponsor, but what platforms have you tested on, and which tests?
>
> Thanks,
> David
>
>>
>> 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