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

Wed Aug 20 22:26:14 UTC 2014

On 8/20/14 9:29 AM, Coleen Phillimore wrote:
> Dan,
> Thanks for taking the time to review this!

No problem. More responses below...

>
> On 8/19/14, 5:39 PM, Daniel D. Daugherty wrote:
>> On 8/15/14 2:18 PM, Coleen Phillimore wrote:
>>> Summary: Use scratch_class to find EMCP methods for breakpoints if 
>>> the old methods are still running
>>>
>>> See bug for more details.  This fix also includes a change to 
>>> nmethod::metadata_do() to not walk the _method multiple times (the 
>>> _method is added to the metadata section of the nmethod), and an 
>>> attempt to help the sweeper clean up these scratch_class instance 
>>> classes sooner.
>>>
>>> Tested with nsk tests, java/lang/instrument tests and jck tests 
>>> (which include some jvmti tests).
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8055008/
>>
>> src/share/vm/oops/instanceKlass.hpp
>>     line 1047   // RedefineClass support
>>         Should be 'RedefineClasses'.
>
> Yes, fixed.
>>
>>     line 1049: void mark_newly_obsolete_methods(Array<Method*>* 
>> old_methods,
>>                int emcp_method_count);
>>         The 'obsolete' part of the function name does not match with
>>         the 'emcp' part of the parameter name. EMCP methods are 'old',
>>         but not 'obsolete'.
>>
>>         Update: OK, I think I get it. We want to mark methods that are
>>         newly transitioning from EMCP -> obsolete and the 
>> emcp_method_count
>>         parameter tells us if there is any work to do.
>
> Yes, it's not used to figure out which ones are EMCP anymore, just to 
> know if there are any.
>>
>> src/share/vm/oops/instanceKlass.cpp
>>     line 3023:  } // pvw is cleaned up
>>         'pvw' no longer exists so comment is stale.
>
> fixed.
>>
>>     line 3455:  // check the previous versions array
>>         This comment should move above this line:
>>
>>         3453     for (; pv_node != NULL; ) {
>>
>>         and 'array' should change to 'list'.
>>
>>         Sorry for the bad placement of the original comment.
>
> ok.
>
>>
>>     line 3463: last->link_previous_versions(pv_node);
>>         So now we've overwritten the previous value of
>>         last->previous_versions. How does that InstanceKlass
>>         get freed? Maybe a short comment?
>
>         // The previous version InstanceKlass is on the 
> ClassLoaderData deallocate list
>         // so will be deallocated during the next phase of class 
> unloading.

Like the new comment.

>
>>     line 3484: // Mark the emcp method as obsolete if it's not executing
>>         I'm not sure about whether this is a good idea. When we had a
>>         redefine make a method obsolete before, we knew that we could
>>         make all older but previously EMCP methods obsolete because
>>         the new method version did make them obsolete.
>>
>>         With this optimization, we're saying that no thread is executing
>>         the method so we're going to make it obsolete even if the 
>> current
>>         redefine did not do so. I worry about a method call that is in
>>         the early stages of assembling the call stack being caught
>>         calling a method that is now obsolete but not because of a
>>         redefine made it obsolete. Just FYI, one of the tracing flags
>>         catches unexpected calls to obsolete methods today and it does
>>         catch the current system's legitimate race.
>
> I need to study this (I think your next email is about this too). I 
> tried to preserve the behavior before but maybe I didn't.

I'll pick this one up in the later reply.

>>
>>     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.
>
> Since is_emcp() means !is_obsolete && !is_deleted, so ineffect I 
> changed that to not include the deleted methods.  So I think it's 
> equivalent and I think I have to account for deleted methods in the 
> previous_version instanceKlasses.

Thanks for setting me straight and I agree you are right here.

>>
>>     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.
>
> Yes, I can change all emcp to EMCP - should I change the is_EMCP() 
> function as well?

Just in the comments (I think). I'm fairly certain that we didn't
use EMCP in code, but I could be wrong...

>>
>>         Also, set what on EMCP methods on the stack?
>
> This comment doesn't make very much sense.  How about:
>
> But I may have to rewrite it because I think your point about
>
>     line 3484: // Mark the emcp method as obsolete if it's not executing

Right, we'll finalize the comment later.

>
>> line 3591: ... If emcp method from
>>     line 3592: // a previous redefinition may be made obsolete by 
>> this redefinition.
>>         Having trouble parsing this comment.
>
>   // Mark newly obsolete methods in remaining previous versions. An 
> EMCP method from
>   // a previous redefinition may be made obsolete by this redefinition.

I like the new 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.
>>
>
> Your comment is better, thanks!  It's nice to have that comment there.

Glad you like it.

>
>> 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)?
>
> Yes. It is added to the metadata_section so didn't need an extra 
> walk.  This is only used for checking code and redefine classes, so 
> it's safe to remove this.

Thanks for confirming. Kinda reminds me of the oop-map stuff... :-)

>
>> src/share/vm/memory/universe.cpp
>>     No comments (resisting 'The Walking Dead' ref...)
>
> Yeah the sematics for zombie nmethods is chilling.

Meant to have a :-) in that comment... :-O

>>
>> 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.
>
> I don't like adding sleeps but you are right, we might not be able to 
> interrupt these threads.

I agree when the sleep() calls are for timing or "correctness".
In this case, they are for not swamping the test machine...

> I'll answer your other mail also.  I think that you are right, we may 
> be making EMCP methods obsolete too soon.
>
> Thanks for the spending time and thinking hard about this.   This is 
> some complicated code.

Do It Yourself brain surgery usually is... :-)

Dan

>
> Coleen
>>
>> Dan
>>
>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8055008
>>>
>>> Thanks,
>>> Coleen
>>
>