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