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

Wed Nov 29 10:04:43 UTC 2017

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>:

> 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/20
>>>>>> 17-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().getTypeD
>>>>>>>>> ataBase()));
>>>>>>>>>      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/20
>>>>>>>>> 17-October/021965.html
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171129/7aca13b4/attachment-0001.html>