RFS(S): 8222934: mark new VM option AllowRedefinitionToAddOrDeleteMethods as deprecated

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Fri Apr 26 18:39:27 UTC 2019 

Looks good to me.
Coleen

On 4/26/19 2:31 PM, serguei.spitsyn at oracle.com wrote:
> Hi Dan,
>
> Thank you for re-review!
> I'll fix the spaces at the end, thanks.
>
> Thanks,
> Serguei
>
> On 4/26/19 08:27, Daniel D. Daugherty wrote:
>> On 4/25/19 10:57 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Coleen and Dan,
>>>
>>> Updated webrev is:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.2/
>>
>> src/hotspot/share/runtime/globals.hpp
>>     No comments.
>>
>> test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java
>>     No comments.
>>
>> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java
>>     L100:     // So, this redefinition will add it back which is 
>> expected to work.
>>         There's a space at the end of this comment line. jcheck may 
>> complain.
>>
>>     L132:     // So, this redefinition will deleate it back which is 
>> expected to work.
>>         Typo: s/deleate it back/delete it/
>>
>>         There's a space at the end of this comment line. jcheck may 
>> complain.
>>
>> Thumbs up.
>>
>> Dan
>>
>>
>>
>>>
>>> This implements the suggestions:
>>>  VMDeprecatedOptions.java:
>>>   - moved the option to the deprecated non-alias flags section
>>>
>>>  TestAddDeleteMethods.java:
>>>   - removed confusion in redefinition string names and added 
>>> comments recommended by Dan
>>>   - always list methods in order: foo, publicFoo, finalFoo, staticFoo
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 4/25/19 2:41 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Coleen,
>>>>
>>>> Thank you a lot for looking at this!
>>>>
>>>>
>>>> On 4/25/19 2:18 PM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>> On 4/25/19 4:19 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Dan,
>>>>>>
>>>>>> Thank you a lot fore reviewing this!
>>>>>>
>>>>>> On 4/25/19 12:40, Daniel D. Daugherty wrote:
>>>>>>> On 4/24/19 6:18 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Please, review fix for:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8222934
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.1/ 
>>>>>>>>
>>>>>>>
>>>>>>> src/hotspot/share/runtime/globals.hpp
>>>>>>>     No comments.
>>>>>>>
>>>>>>> test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java
>>>>>>>     L42:         // deprecated class redefinition flags:
>>>>>>>     L43: {"AllowRedefinitionToAddDeleteMethods", "true"},
>>>>>>>     L44:
>>>>>>>     L45:         // deprecated non-alias flags:
>>>>>>>         I think your new flag entry should have been added to the
>>>>>>>         "deprecated non-alias flags" section. You don't need to
>>>>>>>         call out that this is a "class redefinition" flag.
>>>>>>>
>>>>>>>         The reason for the "// deprecated alias flags (see also 
>>>>>>> aliased_jvm_flags):"
>>>>>>>         section (below what you changed) is because there is more
>>>>>>>         work to do for those flags.
>>>>>>
>>>>>> Okay, I'm not very familiar with this test, will check how to 
>>>>>> change it.
>>>>>>
>>>>>>>
>>>>>>> test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java 
>>>>>>>
>>>>>>>     L94:     public static String ADeleteStaticFoo =
>>>>>>>         This case is deleting both "staticFoo" and "finalFoo".
>>>>>>>         Is that what you really want? If so, then the test case
>>>>>>>         is misnamed.
>>>>>>
>>>>>> I see your confusion here.
>>>>>> The ADeleteStaticFoo is used after the ADeleteFinalFoo.
>>>>>> So, the "finalFoo" has been already deleted before.
>>>>>> Then the ADeleteStaticFoo only deletes the "staticFoo".
>>>>>>
>>>>>> The same was not the case for ADeleteFinalFoo.
>>>>>> It is because the redefinitions with ADeleteFoo and ADeletePublicFoo
>>>>>> are expected to be rejected with UOE.
>>>>>>
>>>>>>>
>>>>>>>     L119     public static String BAddStaticBar =
>>>>>>>         This case is added "staticBar" and "finalBar". Is that what
>>>>>>>         you really want? If so, then the test case is misnamed.
>>>>>>
>>>>>> This one is similar to the above.
>>>>>> The "finalBar" has already been added bythe BAddFinalBar 
>>>>>> redefinition.
>>>>>>
>>>>>> Please, let me know if you are Okay with it as it is or prefer to 
>>>>>> add a comment with clarification.
>>>>>>
>>>>>>>
>>>>>>>     Still a really cool test here!
>>>>>>
>>>>>> The test was initially written by Coleen (thanks, Coleen!)
>>>>>> I've spoiled it a little bit though. :)
>>>>>
>>>>> Hi Serguei,  You added a lot to it, which is taking me a while to 
>>>>> understand.
>>>>>
>>>>> Why did you make class A inherit from Runnable?
>>>>
>>>> In fact, nothing foxy.
>>>> It implements Runnable, not inherits. :)
>>>> There were two steps.
>>>> First was to decide if we there is a point to call methods in the 
>>>> redefined classes A and B.
>>>> You did it with the in the original test version but you made 
>>>> publicFoo to call others.
>>>> So, I decided that it is useful to make sure the methods are 
>>>> executed well after redefinition.
>>>> Then I decided to use another class B for added methods.
>>>> Calling other methods from publicFoo did not work for me.
>>>> I had to generalize it with run() method and then made
>>>> classes A and B to implement Runnable to make it more clear.
>>>>
>>>>
>>>>> Can you maintain order of the function declarations?
>>>>>
>>>>>    78     public static String ADeletePublicFoo =
>>>>>
>>>>> finalFoo should be before staticFoo in this one.
>>>> Nice catch, thanks!
>>>> Will fix it in the webrev update.
>>>>
>>>>>
>>>>> Oh, now I see what Dan is talking about.  In ADeleteStaticFoo, 
>>>>> finalFoo has already been deleted so you didn't want to also test 
>>>>> adding it back.
>>>>
>>>> Right.
>>>>
>>>>>
>>>>> Thank you for enhancing the test.  I guess it's good that it tests 
>>>>> the new option.
>>>>
>>>> Thanks!
>>>> Serguei
>>>>
>>>>>
>>>>> Coleen
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thumbs up. Your call on whether to tweak the test.
>>>>>>
>>>>>> I'll send a VMDeprecatedOptions related update later.
>>>>>>
>>>>>>
>>>>>> Thanks!
>>>>>> Serguei
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>>   David, in review for 
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8222934 suggested:
>>>>>>>>    1. I would have suggested to add "(Deprecated)" to the 
>>>>>>>> description of the new flag in globals.hpp
>>>>>>>>    2. The new flag should have been added to the deprecated VM 
>>>>>>>> options tests.
>>>>>>>>    3. The new test should run in both a positive and negative 
>>>>>>>> mode so that it also checks that the new flag works.
>>>>>>>>
>>>>>>>>   The webrev above implements this suggestion.
>>>>>>>>
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>>   In progress: Submit mach5 run for the updated tests.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190426/75453edd/attachment.html>

More information about the serviceability-dev mailing list