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

Tue Aug 26 20:33:34 UTC 2014

Thank you for another in-depth look at this code.

On 8/26/14, 12:13 PM, Daniel D. Daugherty wrote:
> 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"

Yes, that's better.
>
>     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++) {

I moved it up but adjusted the comment because we're trying to find any 
emcp running methods.

       // At least one method is still running, check for EMCP methods

>
> 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'

yes.   added two "that"s.
>
> 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".
>

OH.  I thought sett was a typo!  Now I get it.  LOL.

> 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'?

I made this 1 so it'd be EMCP but still not sleep a lot.   I could have 
gone with 5.
>
>     line 74:         public static boolean stop = false;
>         This should also be volatile.

Okay, I changed them all.  I think we can't change qualifiers on static 
variables (although I'm going to write another test to see what happens).
>
>     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?

No, the way this harness is written a comment in newB and evenNewerB 
will mess up the new class.  I don't expect comments to have any effect 
whatsoever for EMCPness.
>
>     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?

That is true.   The output I want to see is the print for "infinite".   
Most of this test was for visual verification that the tracing came out 
right but I thought it should be added because it hits all the code and 
the test exists in the repository.

Thanks for the code review again!

Coleen
>
> 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
>>>>>
>>>>
>>>
>>
>