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