RFR 8055008: Clean up code that saves the previous versions of redefined classes
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Aug 20 15:29:36 UTC 2014
Dan,
Thanks for taking the time to review this!
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.
>
> 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.
>
> 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.
>
> 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?
>
> 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
> 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.
>
> 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.
> 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.
>
> src/share/vm/memory/universe.cpp
> No comments (resisting 'The Walking Dead' ref...)
Yeah the sematics for zombie nmethods is chilling.
>
> 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'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.
Coleen
>
> Dan
>
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8055008
>>
>> Thanks,
>> Coleen
>
More information about the serviceability-dev
mailing list