Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
Dan,
Sorry for the big delay in reply...
It is because I did not have my arguments ready. :(
Please, see below.
On 8/20/14 3:37 PM, Daniel D. Daugherty wrote:
On 8/20/14 9:54 AM, Coleen Phillimore wrote:
Hi, it appears that my code is wrong and maybe the existing code is
wrong also. I have a spec question below.
Rely embedded below...
On 8/19/14, 7:52 PM, serguei.spit...@oracle.com wrote:
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(ArrayMethod**
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...
If an EMCP method is not running, should we save it on a previous
version list anyway so that we can make it obsolete if it's redefined
and made obsolete?
Interesting question. The problem with an EMCP method is that it might
not be running right now, but we could have a Java thread that's in
the process of invoking it that is stopped on a safepoint. We resume
the world and the Java thread finishes calling the EMCP method...
It's really hard to catch in-progress uses of jmethodIDs and make
sure that the in-progress use switches from the EMCP method to the
latest version of the method. A rarely seen race, but it does happen...
It has to be invariant that if an EMCP is not running at redefinition time
then it has to be gone and can not become running in the future.
Otherwise, everything becomes overcomplicated.
This also means that non-running EMCP method can never be made obsolete.
The JVMTI spec is clear about the jmethodIDs and obsolete methods:
Obsolete Methods
The functions |RetransformClasses|
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/27/14 5:40 AM, serguei.spit...@oracle.com wrote: On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Running the Serviceability stack of tests with the following -XX:TraceRedefineClasses=NN flags: //0x1000 | 4096 - detect calls to obsolete methods //0x2000 | 8192 - fail a guarantee() in addition to detection will show failures of the guarantee(). I used to do this periodically and then chase down the failures to make sure only the legitimate races were happening. The reason that we had to add these flags was to find all the places where folks were caching methods in the VM. I think the last place I found and fixed was in reflection. Dan Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/27/14 6:54 AM, Daniel D. Daugherty wrote: On 8/27/14 5:40 AM, serguei.spit...@oracle.com wrote: On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Running the Serviceability stack of tests with the following -XX:TraceRedefineClasses=NN flags: //0x1000 | 4096 - detect calls to obsolete methods //0x2000 | 8192 - fail a guarantee() in addition to detection will show failures of the guarantee(). I used to do this periodically and then chase down the failures to make sure only the legitimate races were happening. The reason that we had to add these flags was to find all the places where folks were caching methods in the VM. I think the last place I found and fixed was in reflection. Ok, thanks. We threat this as buggy behavior, right? Thanks, Serguei Dan Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/27/14 12:41 PM, serguei.spit...@oracle.com wrote: On 8/27/14 6:54 AM, Daniel D. Daugherty wrote: On 8/27/14 5:40 AM, serguei.spit...@oracle.com wrote: On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Running the Serviceability stack of tests with the following -XX:TraceRedefineClasses=NN flags: //0x1000 | 4096 - detect calls to obsolete methods //0x2000 | 8192 - fail a guarantee() in addition to detection will show failures of the guarantee(). I used to do this periodically and then chase down the failures to make sure only the legitimate races were happening. The reason that we had to add these flags was to find all the places where folks were caching methods in the VM. I think the last place I found and fixed was in reflection. Ok, thanks. We threat this as buggy behavior, right? Not necessarily. If it's because of a cached method that didn't get updated to the new version, then that's a bug. However, if it's because of a call that is in progress, then we have not considered that a bug in the past. I don't remember the exact details, but the dance is something like this: - jmethodID converted to methodHandle - call to methodHandle prepared: - methodHandle - methodOop - parameters marshalled - methodOop converted to method impl - method called - somewhere in the middle of call sequence the method is redefined - jmethodID and methodHandle are updated to refer to the new method, but we already converted to the methodOop and the underlying method implementation for the final stages of the call - when we've actually called the now obsolete method, the guarantee() mentioned above fires and we have a false positive for the tracing code Of course, now that we don't have methodOops, maybe this doesn't happen anymore. I haven't done a test run with the above flags enabled in quite some time... Dan Thanks, Serguei Dan Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/27/14, 3:33 PM, Daniel D. Daugherty wrote: On 8/27/14 12:41 PM, serguei.spit...@oracle.com wrote: On 8/27/14 6:54 AM, Daniel D. Daugherty wrote: On 8/27/14 5:40 AM, serguei.spit...@oracle.com wrote: On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Running the Serviceability stack of tests with the following -XX:TraceRedefineClasses=NN flags: //0x1000 | 4096 - detect calls to obsolete methods //0x2000 | 8192 - fail a guarantee() in addition to detection will show failures of the guarantee(). I used to do this periodically and then chase down the failures to make sure only the legitimate races were happening. The reason that we had to add these flags was to find all the places where folks were caching methods in the VM. I think the last place I found and fixed was in reflection. Ok, thanks. We threat this as buggy behavior, right? Not necessarily. If it's because of a cached method that didn't get updated to the new version, then that's a bug. However, if it's because of a call that is in progress, then we have not considered that a bug in the past. I don't remember the exact details, but the dance is something like this: - jmethodID converted to methodHandle - call to methodHandle prepared: - methodHandle - methodOop - parameters marshalled - methodOop converted to method impl - method called - somewhere in the middle of call sequence the method is redefined - jmethodID and methodHandle are updated to refer to the new method, but we already converted to the methodOop and the underlying method implementation for the final stages of the call - when we've actually called the now obsolete method, the guarantee() mentioned above fires and we have a false positive for the tracing code Of course, now that we don't have methodOops, maybe this doesn't happen anymore. I haven't done a test run with the above flags enabled in quite some time... Do you mean methodHandle or MethodHandle above? Or java_lang_reflect_Method? The methodOop in little m methodHandles are not updated to refer to the new method, and Method* isn't either, so that really hasn't changed. I'm not following the call sequence above. But yes, I believe we could get into javaCalls::call() with an method, stop for a safepoint, and end up calling an obsolete method. But that obsolete method is considered already running at that stage because the methodHandle logic marks it as such, so it's not considered a bug. I ran the tests with the -XX:TraceRedefineClasses=0x2000 and didn't get the guarantee though, but that doesn't mean much. I'm reading these mails in reverse... Thanks, Coleen Dan Thanks, Serguei Dan Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
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(ArrayMethod**
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
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
Hi, it appears that my code is wrong and maybe the existing code is
wrong also. I have a spec question below.
On 8/19/14, 7:52 PM, serguei.spit...@oracle.com wrote:
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(ArrayMethod**
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...
If an EMCP method is not running, should we save it on a previous
version list anyway so that we can make it obsolete if it's redefined
and made obsolete?
Currently we don't save previous versions of methods that are not
running. We didn't before permgen elimination either. If GC didn't
find the EMCP method, we would remove the entry.
Thanks,
Coleen
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)
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/20/14 8:54 AM, Coleen Phillimore wrote:
Hi, it appears that my code is wrong and maybe the existing code is
wrong also. I have a spec question below.
On 8/19/14, 7:52 PM, serguei.spit...@oracle.com wrote:
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(ArrayMethod**
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...
If an EMCP method is not running, should we save it on a previous
version list anyway so that we can make it obsolete if it's redefined
and made obsolete?
I hope, Dan will catch me if I'm wrong...
I think, we should not.
An EMCP method can not be made obsolete if it is not running.
BTW, I'm reviewing the webrev too, but probably it'd be better to switch
to updated webrev after it is ready.
Thanks,
Serguei
Currently we don't save previous versions of methods that are not
running. We didn't before permgen elimination either. If GC didn't
find the EMCP method, we would remove the entry.
Thanks,
Coleen
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
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
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(ArrayMethod**
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
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/20/14 9:54 AM, Coleen Phillimore wrote:
Hi, it appears that my code is wrong and maybe the existing code is
wrong also. I have a spec question below.
Rely embedded below...
On 8/19/14, 7:52 PM, serguei.spit...@oracle.com wrote:
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(ArrayMethod**
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...
If an EMCP method is not running, should we save it on a previous
version list anyway so that we can make it obsolete if it's redefined
and made obsolete?
Interesting question. The problem with an EMCP method is that it might
not be running right now, but we could have a Java thread that's in
the process of invoking it that is stopped on a safepoint. We resume
the world and the Java thread finishes calling the EMCP method...
It's really hard to catch in-progress uses of jmethodIDs and make
sure that the in-progress use switches from the EMCP method to the
latest version of the method. A rarely seen race, but it does happen...
Currently we don't save previous versions of methods that are not
running. We didn't before permgen elimination either. If GC didn't
find the EMCP method, we would remove the entry.
Not quite true for the pre-PermGen-Removal (PGR) world. We used to
save weak refs for all of the EMCP methods in the previous version
info. As the EMCP methods became collectible we removed them from
the previous version info. This means if GC could find the EMCP
method anywhere (stack, jmethodID, JNI handle, etc), then it stayed
alive. This means that even if no thread was currently executing
an EMCP method,
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/20/14, 6:37 PM, Daniel D. Daugherty wrote:
On 8/20/14 9:54 AM, Coleen Phillimore wrote:
Hi, it appears that my code is wrong and maybe the existing code is
wrong also. I have a spec question below.
Rely embedded below...
Also embedded reply but I cut some stuff out.
...
If an EMCP method is not running, should we save it on a previous
version list anyway so that we can make it obsolete if it's redefined
and made obsolete?
Interesting question. The problem with an EMCP method is that it might
not be running right now, but we could have a Java thread that's in
the process of invoking it that is stopped on a safepoint. We resume
the world and the Java thread finishes calling the EMCP method...
It's really hard to catch in-progress uses of jmethodIDs and make
sure that the in-progress use switches from the EMCP method to the
latest version of the method. A rarely seen race, but it does happen...
Yes, there may be small cases where EMCP methods could be brought back
to life, possibly with jmethodIDs. Although the combination of changing
Method* in the cpCache for the interpreter, making old methods
non-entrant and deoptimized, and replacing jmethodIDs should prevent it
mostly if not completely. I wouldn't be surprised if there's leakage though.
Currently we don't save previous versions of methods that are not
running. We didn't before permgen elimination either. If GC didn't
find the EMCP method, we would remove the entry.
Not quite true for the pre-PermGen-Removal (PGR) world. We used to
save weak refs for all of the EMCP methods in the previous version
info. As the EMCP methods became collectible we removed them from
the previous version info. This means if GC could find the EMCP
method anywhere (stack, jmethodID, JNI handle, etc), then it stayed
alive. This means that even if no thread was currently executing
an EMCP method, an in-progress call to that method could still
complete and poof now we have the EMCP method back on a stack
somewhere...
True. The case I was worried about is if an EMCP method is made
obsolete by redefinition but we don't have a pointer to it anywhere
because it's not running or referenced, so we can't mark it obsolete.
In this case I guess you can't call the isMethodObsolete() function on
it. I think I went down a rabbit hole.
Thanks,
Coleen
Dan
Thanks,
Coleen
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
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/20/14, 6:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. Yes, this was my error in the change. This is why I made things obsolete if they were not running. I think I can't reuse this flag. My latest changes add a new explicit flag (which we have space for in Method*). In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Our on_stack marking is supposed to look at all the places where GC used to look so I think we can use on_stack to track the lifecycle of EMCP methods. If the EMCP method is somewhere, we will find it! I'm running tests on the latest change, but am also waiting for confirmation from Roland because we were only cleaning out MethodData for EMCP methods and not for running obsolete methods and I think we need to do that for obsolete methods also, which my change does now. I think it was a bug. Thanks Dan for remembering all of this for me! Coleen BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
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(ArrayMethod**
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
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
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(ArrayMethod**
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
RFR 8055008: Clean up code that saves the previous versions of redefined classes
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/ bug link https://bugs.openjdk.java.net/browse/JDK-8055008 Thanks, Coleen
