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

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
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.

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   }

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


Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
Hi Chris,

2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
> 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


> 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 .
I like it because it is simple.

I will change it to traditional style if other reviewer(s) suggest it.


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
>
>
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:

> Hi Chris,
>
> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>> 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
>>
>>

Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
In reply to this post by Yasumasa Suenaga-4
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.
-------------

Thanks,

Yasumasa


2017-11-29 10:09 GMT+09:00 Chris Plummer <[hidden email]>:

> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>
>> Hi Chris,
>>
>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>>>
>>> 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
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
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 <[hidden email]>:
>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>> Hi Chris,
>>>
>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>>>> 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
>>>>
>>>>

Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
Hi Chris,

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

Yes. I think we should avoid error(s) in the future about changing
const value(s) in HotSpot.
They are difficult to catch on run-time.

So I send review request.


Thanks,

Yasumasa


2017-11-29 11:26 GMT+09:00 Chris Plummer <[hidden email]>:

> 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 <[hidden email]>:
>>>
>>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>>>
>>>> Hi Chris,
>>>>
>>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>>>>>
>>>>> 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
>>>>>
>>>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Jini George
In reply to this post by Chris Plummer
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.

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 <[hidden email]>:
>>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>>> Hi Chris,
>>>>
>>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>>>>> 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 
>>>>>
>>>>>
>>>>>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
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 <[hidden email]>:
>>>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>>>> Hi Chris,
>>>>>
>>>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>>>>>> 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 
>>>>>>
>>>>>>
>>>>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
In reply to this post by Jini George
Hi Jini,

Ok, that's all I needed to hear. Just wanted to make sure the disruption
was being considered.

thanks,

Chris

On 11/28/17 10: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.
>
> 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 <[hidden email]>:
>>>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>>>> Hi Chris,
>>>>>
>>>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>>>>>> 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 
>>>>>>
>>>>>>
>>>>>>
>>


Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
In reply to this post by David Holmes
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/


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


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 <[hidden email]>:

> 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 <[hidden email]>:
>>>>>
>>>>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>>>>>
>>>>>> Hi Chris,
>>>>>>
>>>>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
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 <[hidden email]>:
>> 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 <[hidden email]>:
>>>>>>
>>>>>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>>>>>>
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:
>>>>>>>>
>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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" <[hidden email]>:
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 <[hidden email]>:
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 <[hidden email]>:

On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:

Hi Chris,

2017-11-29 5:32 GMT+09:00 Chris Plummer <[hidden email]>:

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




Reply | Threaded
Open this post in threaded view
|

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

Jini George
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" <[hidden email]
> <mailto:[hidden email]>>:
>
>     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 <[hidden email]
>         <mailto:[hidden email]>>:
>
>             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
>                         <[hidden email]
>                         <mailto:[hidden email]>>:
>
>
>                             On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>
>
>                                 Hi Chris,
>
>                                 2017-11-29 5:32 GMT+09:00 Chris Plummer
>                                 <[hidden email]
>                                 <mailto:[hidden email]>>:
>
>
>                                     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>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Yasumasa Suenaga-4
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" <[hidden email]
> <mailto:[hidden email]>>:
>
>     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 <[hidden email]
>         <mailto:[hidden email]>>:
>
>             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
>                         <[hidden email]
>                         <mailto:[hidden email]>>:
>
>
>                             On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>
>
>                                 Hi Chris,
>
>                                 2017-11-29 5:32 GMT+09:00 Chris Plummer
>                                 <[hidden email]
>                                 <mailto:[hidden email]>>:
>
>
>                                     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>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|

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

Yasumasa Suenaga-4
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 <[hidden email]>:

> 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" <[hidden email]
>> <mailto:[hidden email]>>:
>>
>>
>>     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 <[hidden email]
>>         <mailto:[hidden email]>>:
>>
>>
>>             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
>>                         <[hidden email]
>>                         <mailto:[hidden email]>>:
>>
>>
>>                             On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>
>>
>>                                 Hi Chris,
>>
>>                                 2017-11-29 5:32 GMT+09:00 Chris Plummer
>>                                 <[hidden email]
>>                                 <mailto:[hidden email]>>:
>>
>>
>>
>>                                     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>
>>
>>
>>
>>
>