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

[Daniel D. Daugherty]

On 8/19/14 3: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'.
>
>     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.
>
> src/share/vm/oops/instanceKlass.cpp
>     line 3023:  } // pvw is cleaned up
>         'pvw' no longer exists so comment is stale.
>
>     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.
>
>     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?
>
>     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.

JVM/TI has an isMethodObsolete() API:

     jvmtiError
     IsMethodObsolete(jvmtiEnv* env,
                 jmethodID method,
                 jboolean* is_obsolete_ptr)

It would be possible for an agent to observe a method changing from
not obsolete to obsolete when that's not expected. I suspect that
this would be a spec violation.

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