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

[Daniel D. Daugherty]

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.

     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