On 29/11/2017 4:19 PM, Jini George wrote:
Hi Chris,
Thank you for raising this. I agree it is disruptive, but we are trying
to address the issue of frequent SA breakages with hotspot changes, and
the causes of these breakages. One of the reasons is the redefinition of
constants, which is extremely error prone. There have been multiple
cases where the changes get done in hotspot and the corresponding
changes get inadvertently missed out in SA. And this does not get
caught, sometimes, for months. I believe that the switch case statements
conversion to if-else statements is not a heavy price to pay for
avoiding errors like these.
I agree it is good to ensure this always matches the VM. I also agree it
is unfortunate we lost the ability to keep the switch statements - so be
it. I'm more concerned that the BasicType static fields are no longer
final (that may raise parfait warnings!). That can be fixed using a
no-arg constructor for static initialization and adding a private
setType method used for the real initialization.
I'm not at all clear why we need the tXxx and T_XXX forms? The former
can be obtained from the latter. And with the change to use the
getTXxx() functions I think we can actually do away with all the tXxx
static fields. The getTXxx() functions can be implemented as "return
T_XXX.getType(); and the intToBasicType() function can also examine
getType(). The name could be stored as a field, set at construction
time. Just a thought. :)
Thanks,
David
Thanks!
- Jini.
On 11/29/2017 7:56 AM, Chris Plummer wrote:
On 11/28/17 5:23 PM, Yasumasa Suenaga wrote:
Hi Chris,
I understood the reason for getting rid of the case statements. I
was just
wondering if you weighed this code disruption vs. the value of what
you are
fixing.
Jini has pointed it as below and I agree with him:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
-------------
One point I want to make is that we have the
enum BasicTypeSize redefined in SA as public static final values, and
this makes it error prone when existing enum values change, just as in
this case. An ideal solution would be to include this in vmStructs.cpp
as a declare_constant() macro, and read this in SA with the
db.lookupIntConstant() method.
-------------
Hi Yasumasa,
Yes, I had read that and understand the point being made. What I'm
asking is now that you've implemented it and seen the disruption to
the switch statements (which I assume you and Jini were not aware of
before embarking on this), is it still worth doing? It's not really
that big of a deal to me. I just want to make sure it's been taken
into consideration.
thanks,
Chris
Thanks,
Yasumasa
2017-11-29 10:09 GMT+09:00 Chris Plummer <chris.plum...@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.plum...@oracle.com>:
Hi Yasumasa,
This isn't code I know very well, and I'm not a Reviewer. Just a
couple
of
observations.
I wonder if the person who originally suggested this change
realized the
disruption it would have to existing switch statements. I'm not
saying
the
change shouldn't be done for this reason, but it is something to
at least
consider.
According to JLS, `case` label need to have constant expression.
We cannot set `static final` to the field which is set at
initialize().
https://docs.oracle.com/javase/specs/jls/se9/html/jls-14.html#jls-14.11
I understood the reason for getting rid of the case statements. I
was just
wondering if you weighed this code disruption vs. the value of what
you are
fixing.
Your coding pattern for the following differs from the existing 200+
instances of VM.registerVMInitializedObserver() calls already in
place. I
suggest you be consistent with existing code.
71 static {
72 VM.registerVMInitializedObserver(
73 (o, d) -> initialize(VM.getVM().getTypeDataBase()));
74 }
This style has been used in JavaThreadsPanel.java .
Ah, I missed that one case, but then it's one that you added. :)
I like it because it is simple.
I will change it to traditional style if other reviewer(s) suggest it.
I think consistency is important.
thanks,
Chris
Thanks,
Yasumasa
thanks,
Chris
On 11/27/17 11:49 PM, Yasumasa Suenaga wrote:
Hi all,
Enum values in BasicType and BasicTypeSize are declared as const
values. However, it makes error prone when existing enum values
change.
They should refer to HotSpot values via VMStructs.
This issue has been pointed by Jini [1].
I uploaded webrev for this issue. Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.00/
I cannot access JPRT. So I need a sponsor.
Thanks,
Yasumasa
[1]
http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
