RFR: 8190837: BasicType and BasicTypeSize should refer to HotSpot values
David Holmes
david.holmes at oracle.com
Wed Nov 29 09:58:08 UTC 2017
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/
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
>
>
> 2017-11-29 16:01 GMT+09:00 David Holmes <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
>>>>> -------------
>>>>> 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>:
>>>>>>
>>>>>> 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>:
>>>>>>>>
>>>>>>>> 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
>>>>>>
>>>>>> 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/
>>>>>>>>
>>>>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> [1]
>>>>>>>>
>>>>>>>>
>>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
>>>>>>>>
>>>>>>>>
>>>>
>>
More information about the serviceability-dev
mailing list