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

[serguei.spit...@oracle.com]

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(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...

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| 
<http://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#RetransformClasses> 
and |RedefineClasses| 
<http://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#RedefineClasses> 
can cause new versions of methods to be installed. An original version 
of a method is considered equivalent to the new version if:

 * their bytecodes are the same except for indices into the constant
   pool and
 * the referenced constants are equal.

An original method version which is not equivalent to the new method 
version is called obsolete and is assigned a new method ID; the original 
method ID now refers to the new method version. A method ID can be 
tested for obsolescence with |IsMethodObsolete| 
<http://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#IsMethodObsolete>. 



From the above, it seems that the EMCP methods do not get new jmethodIDs.
It means that any use of jmethodID that referred to the EMCP method 
before the redefinition
after redefinition should refer to the new (non-EMCP) method version.
We must treat it as a bug if this invariant is broken.

However, if an EMCP method is already running (there is a frame on the 
stack)
then it can not be replaced with the new method version.
Next redefinition can make such EMCP method to become obsolete, and so,
it should get new methodID if it is still running.



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...

Do we have any test that demonstrate this behavior?
Should we treat this behavior as a bug?
If we decide to treat it as a bug then is it possible to fix such issues?


Thanks,
Serguei



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