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

Wed, 20 Aug 2014 18:22:28 -0700 


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



































					Reply via email to