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

[Coleen Phillimore]

On 8/20/14, 6:37 PM, Daniel D. Daugherty wrote:
> On 8/20/14 9:54 AM, Coleen Phillimore wrote:
>>
>> Hi, it appears that my code is wrong and maybe the existing code is 
>> wrong also.  I have a spec question below.
>
> Rely embedded below...

Also embedded reply but I cut some stuff out.
> ...
>> 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?
>
> Interesting question. The problem with an EMCP method is that it might
> not be running right now, but we could have a Java thread that's in
> the process of invoking it that is stopped on a safepoint. We resume
> the world and the Java thread finishes calling the EMCP method...
>
> It's really hard to catch in-progress uses of jmethodIDs and make
> sure that the in-progress use switches from the EMCP method to the
> latest version of the method. A rarely seen race, but it does happen...
>

Yes, there may be small cases where EMCP methods could be brought back 
to life, possibly with jmethodIDs.  Although the combination of changing 
Method* in the cpCache for the interpreter, making old methods 
non-entrant and deoptimized, and replacing jmethodIDs should prevent it 
mostly if not completely. I wouldn't be surprised if there's leakage though.
>
>> Currently we don't save previous versions of methods that are not 
>> running.  We didn't before permgen elimination either.  If GC didn't 
>> find the EMCP method, we would remove the entry.
>
> Not quite true for the pre-PermGen-Removal (PGR) world. We used to
> save weak refs for all of the EMCP methods in the previous version
> info. As the EMCP methods became collectible we removed them from
> the previous version info. This means if GC could find the EMCP
> method anywhere (stack, jmethodID, JNI handle, etc), then it stayed
> alive. This means that even if no thread was currently executing
> an EMCP method, an in-progress call to that method could still
> complete and poof now we have the EMCP method back on a stack
> somewhere...
>

True.  The case I was worried about is if an EMCP method is made 
obsolete by redefinition but we don't have a pointer to it anywhere 
because it's not running or referenced, so we can't mark it obsolete.  
In this case I guess you can't call the isMethodObsolete() function on 
it.   I think I went down a rabbit hole.

Thanks,
Coleen

> Dan
>
>
>>
>> Thanks,
>> Coleen
>>
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> Dan
>>>>
>>>>
>>>>>
>>>>>     line 3527: // clear out any matching EMCP method entries the 
>>>>> hard way.
>>>>>         Perhaps "mark" instead of "clear out"?
>>>>>
>>>>>     old line 3659: if (!method->is_obsolete() &&
>>>>>     new line 3546: if (method->is_emcp() &&
>>>>>         The old code is correct. The old code won't remark a 
>>>>> method that
>>>>>         was already made obsolete so there won't be more than one 
>>>>> trace
>>>>>         message for that operation.
>>>>>
>>>>>     line 3581: // stack, and set emcp methods on the stack.
>>>>>         In comments 'emcp' should be 'EMCP'. We did not do that in 
>>>>> the
>>>>>         code because of HotSpot's variable name style.
>>>>>
>>>>>         Also, set what on EMCP methods on the stack?
>>>>>
>>>>>     line 3591: ... If emcp method from
>>>>>     line 3592: // a previous redefinition may be made obsolete by 
>>>>> this redefinition.
>>>>>         Having trouble parsing this comment.
>>>>>
>>>>> src/share/vm/oops/method.hpp
>>>>>     line 693: // emcp methods (equivalent method except constant 
>>>>> pool is different)
>>>>>     line 694: // that are old but not obsolete or deleted.
>>>>>         Perhaps:
>>>>>
>>>>>         // EMCP methods are old but not obsolete or deleted. 
>>>>> Equivalent
>>>>>         // Modulo Constant Pool means the method is equivalent except
>>>>>         // the constant pool and instructions that access the 
>>>>> constant
>>>>>         // pool might be different.
>>>>>
>>>>> src/share/vm/prims/jvmtiImpl.cpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/prims/jvmtiRedefineClasses.cpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/code/nmethod.cpp
>>>>>     So in the original code f(_method) was being called two extra
>>>>>     times? (once in the while-loop and once at the end) So I'm
>>>>>     guessing that f(_method) is properly called when the rest of
>>>>>     the metadata is handled in the nmethod (line 2085)?
>>>>>
>>>>> src/share/vm/memory/universe.cpp
>>>>>     No comments (resisting 'The Walking Dead' ref...)
>>>>>
>>>>> test/runtime/RedefineTests/RedefineFinalizer.java
>>>>>     No comments.
>>>>>
>>>>> test/runtime/RedefineTests/RedefineRunningMethods.java
>>>>>     line 44:  "       while (!stop) { count2++; }" +
>>>>>     line 53:  while (!stop) { count1++; }
>>>>>     line 56:  while (!stop) { count2++; }
>>>>>
>>>>>         These may not behave well on OSes with bad threading
>>>>>         models. You might want to add a helper function that
>>>>>         sleeps for 10ms and have each of these loops call it
>>>>>         so the test more well behaved.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8055008
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140820/cfd53425/attachment.html>