RFR 8055008: Clean up code that saves the previous versions of redefined classes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Aug 19 23:52:59 UTC 2014
On 8/19/14 3:53 PM, Daniel D. Daugherty wrote:
>
> 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.
I agree that this looks like a spec violation.
The emcp methods by definition are non-obsolete,
or in opposite, the obsolete methods are non-emcp:
http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#obsoleteMethods
Old method versions may become obsolete, not emcp:
http://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#RedefineClasses
But maybe I'm missing something...
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/20140819/4c314c21/attachment.html>
More information about the serviceability-dev
mailing list