RFR 8055008: Clean up code that saves the previous versions of redefined classes

[Daniel D. Daugherty]

On 8/22/14 8:26 AM, Coleen Phillimore wrote:
>
> Thanks Dan, Serguei and Roland for discussion on this change.  The 
> latest version is here:
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8055008_2/

src/share/vm/oops/instanceKlass.hpp
     No comments.

src/share/vm/oops/instanceKlass.cpp
     line 3616:           // we add breakpoints for it.
         "we add breakpoints" -> "we can add breakpoints"

     line 3618:           // At least one emcp method is still running
         Do we still need this comment line? Or move it between these
         lines:

         line 3611:     } else {
                          // At least one EMCP method is still running
         line 3612:       for (int i = 0; i < old_methods->length(); i++) {

src/share/vm/oops/method.hpp
     No comments.

src/share/vm/prims/jvmtiRedefineClasses.hpp
     line 407:   // counts the number of methods are EMCP (Equivalent 
Module Constant Pool).
         Typo: 'methods are EMCP' -> 'methods that are EMCP'

src/share/vm/prims/jvmtiRedefineClasses.cpp
     line 3381:   // track number of methods are EMCP for 
add_previous_version() call below
         Typo: 'methods are EMCP' -> 'methods that are EMCP'

src/share/vm/prims/jvmtiImpl.cpp
     line 304:           meth_act == &Method::set_breakpoint ? "set" : 
"clear",
         Typo: "set" should be "sett" so that the output is "setting"
             instead of "seting".

src/share/vm/code/nmethod.cpp
     No comments.

src/share/vm/memory/universe.cpp
     No comments.

test/runtime/RedefineTests/RedefineFinalizer.java
     No comments.

test/runtime/RedefineTests/RedefineRunningMethods.java
     line 61:                 " Thread.currentThread().sleep(1);" +
         Should the '1' be '10'?

     line 74:         public static boolean stop = false;
         This should also be volatile.

     line 77:             Thread.currentThread().sleep(10);//sleep for 10 ms
         So the original has a comment, but 'newB' and 'evenNewerB' don't
         have a comment. Are you testing to see the effect of a comment
         change on the EMCP'ness of the resulting redefine?

     line 109:         B.infinite();
         So the idea with this call, is that if the redefine to "newB"
         doesn't work right, the test will hang in this call. Do I have
         this right?

Dan



>
> Changes from the last version (don't have the setup to do a diff 
> webrev, sorry) are that I have a new flag to mark running emcp methods 
> so we can set breakpoints in only those.  Also, confirmed that we need 
> to clean_weak_method_links in obsolete methods too. Made changes per 
> review comments.  Also, added more to the test so that all tracing in 
> InstanceKlass comes out.   Reran all tests (nsk, jck, jtreg, 
> java/lang/instrument).
>
> Thanks to whoever made command line processing handle hex numbers!
>
> Thanks,
> Coleen
>
> On 8/20/14, 9:26 PM, Coleen Phillimore wrote:
>>
>> On 8/20/14, 6:45 PM, Daniel D. Daugherty wrote:
>>> On 8/20/14 2:01 PM, Coleen Phillimore wrote:
>>>> On 8/20/14, 3:49 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>
>>>>>> If an EMCP method is not running, should we save it on a previous 
>>>>>> version list anyway so that we can make it obsolete if it's 
>>>>>> redefined and made obsolete?
>>>>>
>>>>> I hope, Dan will catch me if I'm wrong...
>>>>>
>>>>> I think, we should not.
>>>>> An EMCP method can not be made obsolete if it is not running.
>>>>>
>>>>
>>>>
>>>> It should be this way otherwise we'd have to hang onto things forever.
>>>
>>> An EMCP method should only be made obsolete if a RedefineClasses() or
>>> RetransformClasses() operation made it so. We should not be leveraging
>>> off the obsolete-ness attribute to solve a life-cycle problem.
>>
>> Yes, this was my error in the change.  This is why I made things 
>> obsolete if they were not running.  I think I can't reuse this flag.  
>> My latest changes add a new explicit flag (which we have space for in 
>> Method*).
>>>
>>> In the pre-PGR world, we could trust GC to make a completely unused
>>> EMCP method collectible and eventually our weak reference would go
>>> away. Just because an EMCP method is not on a stack does not mean
>>> that it is not used so we need a different way to determine whether
>>> it is OK to no longer track an EMCP method.
>>
>> Our on_stack marking is supposed to look at all the places where GC 
>> used to look so I think we can use on_stack to track the lifecycle of 
>> EMCP methods.  If the EMCP method is somewhere, we will find it!
>>
>> I'm running tests on the latest change, but am also waiting for 
>> confirmation from Roland because we were only cleaning out MethodData 
>> for EMCP methods and not for running obsolete methods and I think we 
>> need to do that for obsolete methods also, which my change does now.  
>> I think it was a bug.
>>
>> Thanks Dan for remembering all of this for me!
>>
>> Coleen
>>>
>>>
>>>>
>>>>> BTW, I'm reviewing the webrev too, but probably it'd be better to 
>>>>> switch to updated webrev after it is ready.
>>>>
>>>> Yes, this is a good idea.  I figured out why I made emcp methods 
>>>> obsolete, and I'm fixing that as well as Dan's comments. Thanks!
>>>
>>> Cool! I'm looking forward to the next review.
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Coleen
>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>
>>>
>>
>