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

[serguei.spitsyn at oracle.com]

Hi Coleen,


src/share/vm/code/nmethod.cpp

   Nice simplification.


src/share/vm/memory/universe.cpp

   No comments


src/share/vm/oops/instanceKlass.cpp

   A minor question about two related fragments:

3505       // next previous version
3506       last = pv_node;
3507       pv_node = pv_node->previous_versions();
3508       version++;

   Should the version be incremented to the case 3462-3469 as at the 
line 3508?
   It is not a big issue as the version number is used at the RC_TRACE 
line only:
      3496               method->signature()->as_C_string(), j, version));


   We still have no consensus on the following question:
   Can a non-running EMCP method become running again after the flag was 
cleared?

3487           if (!method->on_stack()) {
3488             if (method->is_running_emcp()) {
3489               method->set_running_emcp(false);  // no breakpoints 
for non-running methods
3490             }

   Just wanted to be sure what is the current view on this. :)


src/share/vm/oops/instanceKlass.hpp

   No comments


src/share/vm/oops/method.hpp

   Just some questions.
   Usefulness of this new function depends on basic ability of a 
non-running method to become running again:
        is_running_emcp()

   The questions are:
    - How precise is the control of this bit?
    - Should we clear this bit after all method invocations have been 
finished?
    - Can a EMCP method become running again after the bit was cleared 
or not set?


src/share/vm/prims/jvmtiImpl.cpp

   300       if (method->is_running_emcp() &&

    Is it possible that an EMCP method becomes running after the bit 
is_running_emcp() is set?
     Do we miss breakpoints in such a case?


  303         RC_TRACE(0x00000800, ("%sing breakpoint in %s(%s)",
  304           meth_act == &Method::set_breakpoint ? "set" : "clear",

    The change from  "sett" to "set" seems to be wrong (see the line 303):


src/share/vm/prims/jvmtiRedefineClasses.cpp
src/share/vm/prims/jvmtiRedefineClasses.hpp

   No comments


Thanks,
Serguei


On 8/22/14 7: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/
>
> 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
>>>>
>>>
>>
>