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 <david.hol...@oracle.com>: > 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" <david.hol...@oracle.com >> <mailto:david.hol...@oracle.com>>: >> >> >> Hi Yasumasa, >> >> On 29/11/2017 6:42 PM, Yasumasa Suenaga wrote: >> >> Hi David, >> >> That can be fixed using a no-arg >> constructor for static initialization and adding a private >> setType method >> used for the real initialization. >> >> >> I uploaded new webrev. Is it okay? >> >> http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.01/ >> <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 <david.hol...@oracle.com >> <mailto:david.hol...@oracle.com>>: >> >> >> On 29/11/2017 4:19 PM, Jini George wrote: >> >> >> Hi Chris, >> >> Thank you for raising this. I agree it is disruptive, >> but we are trying to >> address the issue of frequent SA breakages with hotspot >> changes, and the >> causes of these breakages. One of the reasons is the >> redefinition of >> constants, which is extremely error prone. There have >> been multiple cases >> where the changes get done in hotspot and the >> corresponding changes get >> inadvertently missed out in SA. And this does not get >> caught, sometimes, for >> months. I believe that the switch case statements >> conversion to if-else >> statements is not a heavy price to pay for avoiding >> errors like these. >> >> >> >> I agree it is good to ensure this always matches the VM. I >> also agree it is >> unfortunate we lost the ability to keep the switch >> statements - so be it. >> I'm more concerned that the BasicType static fields are no >> longer final >> (that may raise parfait warnings!). That can be fixed using >> a no-arg >> constructor for static initialization and adding a private >> setType method >> used for the real initialization. >> >> I'm not at all clear why we need the tXxx and T_XXX forms? >> The former can be >> obtained from the latter. And with the change to use the >> getTXxx() functions >> I think we can actually do away with all the tXxx static >> fields. The >> getTXxx() functions can be implemented as "return >> T_XXX.getType(); and the >> intToBasicType() function can also examine getType(). The >> name could be >> stored as a field, set at construction time. Just a thought. >> :) >> >> Thanks, >> David >> >> >> Thanks! >> - Jini. >> >> On 11/29/2017 7:56 AM, Chris Plummer wrote: >> >> >> On 11/28/17 5:23 PM, Yasumasa Suenaga wrote: >> >> >> Hi Chris, >> >> I understood the reason for getting rid of >> the case statements. I was >> just >> wondering if you weighed this code >> disruption vs. the value of what you >> are >> fixing. >> >> >> Jini has pointed it as below and I agree with him: >> >> >> >> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html >> >> <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 >> <mailto: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 >> <mailto: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 >> >> <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> >> >> >> >> >
