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]>: > 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 <[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/20 >>>>>> 17-October/021965.html >>>>>> ------------- >>>>>> One point I want to make is that we have the >>>>>> enum BasicTypeSize redefined in SA as public static final values, and >>>>>> this makes it error prone when existing enum values change, just as in >>>>>> this case. An ideal solution would be to include this in vmStructs.cpp >>>>>> as a declare_constant() macro, and read this in SA with the >>>>>> db.lookupIntConstant() method. >>>>>> ------------- >>>>>> >>>>> >>>>> Hi Yasumasa, >>>>> >>>>> Yes, I had read that and understand the point being made. What I'm >>>>> asking >>>>> is now that you've implemented it and seen the disruption to the switch >>>>> statements (which I assume you and Jini were not aware of before >>>>> embarking >>>>> on this), is it still worth doing? It's not really that big of a deal >>>>> to me. >>>>> I just want to make sure it's been taken into consideration. >>>>> >>>>> thanks, >>>>> >>>>> Chris >>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Yasumasa >>>>>> >>>>>> >>>>>> 2017-11-29 10:09 GMT+09:00 Chris Plummer <[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]>: >>>>>>>> >>>>>>>>> >>>>>>>>> Hi Yasumasa, >>>>>>>>> >>>>>>>>> This isn't code I know very well, and I'm not a Reviewer. Just a >>>>>>>>> couple >>>>>>>>> of >>>>>>>>> observations. >>>>>>>>> >>>>>>>>> I wonder if the person who originally suggested this change >>>>>>>>> realized >>>>>>>>> the >>>>>>>>> disruption it would have to existing switch statements. I'm not >>>>>>>>> saying >>>>>>>>> the >>>>>>>>> change shouldn't be done for this reason, but it is something to at >>>>>>>>> least >>>>>>>>> consider. >>>>>>>>> >>>>>>>> >>>>>>>> According to JLS, `case` label need to have constant expression. >>>>>>>> We cannot set `static final` to the field which is set at >>>>>>>> initialize(). >>>>>>>> >>>>>>>> >>>>>>>> https://docs.oracle.com/javase/specs/jls/se9/html/jls-14. >>>>>>>> html#jls-14.11 >>>>>>>> >>>>>>> >>>>>>> I understood the reason for getting rid of the case statements. I was >>>>>>> just >>>>>>> wondering if you weighed this code disruption vs. the value of what >>>>>>> you >>>>>>> are >>>>>>> fixing. >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Your coding pattern for the following differs from the existing 200+ >>>>>>>>> instances of VM.registerVMInitializedObserver() calls already in >>>>>>>>> place. I >>>>>>>>> suggest you be consistent with existing code. >>>>>>>>> >>>>>>>>> 71 static { >>>>>>>>> 72 VM.registerVMInitializedObserver( >>>>>>>>> 73 (o, d) -> initialize(VM.getVM().getTypeD >>>>>>>>> ataBase())); >>>>>>>>> 74 } >>>>>>>>> >>>>>>>> >>>>>>>> This style has been used in JavaThreadsPanel.java . >>>>>>>> >>>>>>> >>>>>>> Ah, I missed that one case, but then it's one that you added. :) >>>>>>> >>>>>>>> >>>>>>>> I like it because it is simple. >>>>>>>> >>>>>>>> I will change it to traditional style if other reviewer(s) suggest >>>>>>>> it. >>>>>>>> >>>>>>> >>>>>>> I think consistency is important. >>>>>>> >>>>>>> thanks, >>>>>>> >>>>>>> Chris >>>>>>> >>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Yasumasa >>>>>>>> >>>>>>>> >>>>>>>> thanks, >>>>>>>>> >>>>>>>>> Chris >>>>>>>>> >>>>>>>>> >>>>>>>>> On 11/27/17 11:49 PM, Yasumasa Suenaga wrote: >>>>>>>>> >>>>>>>>> Hi all, >>>>>>>>> >>>>>>>>> Enum values in BasicType and BasicTypeSize are declared as const >>>>>>>>> values. However, it makes error prone when existing enum values >>>>>>>>> change. >>>>>>>>> They should refer to HotSpot values via VMStructs. >>>>>>>>> >>>>>>>>> This issue has been pointed by Jini [1]. >>>>>>>>> >>>>>>>>> I uploaded webrev for this issue. Could you review it? >>>>>>>>> >>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.00/ >>>>>>>>> >>>>>>>>> I cannot access JPRT. So I need a sponsor. >>>>>>>>> >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> >>>>>>>>> Yasumasa >>>>>>>>> >>>>>>>>> >>>>>>>>> [1] >>>>>>>>> >>>>>>>>> >>>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/20 >>>>>>>>> 17-October/021965.html >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>> >>>
