JVM/TI code review request (XS and M) (7182152) (original) (raw)

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Feb 4 16:19:54 PST 2013

On 2/4/13 4:17 PM, Coleen Phillimore wrote:

Hi Dan,

I think this version looks good.

Thanks!

From your other reply:

On 2/4/2013 11:40 AM, Daniel D. Daugherty wrote: is present in the HSX-23.6, HSX-24, and HSX-25 versions of isinterestingmethodentry(), but is not used by the original checknooldentries() or any of the new checknooldorobsoleteentries(). I'll put that info in the new bug to track the future work. what is this new bug? It would be nice if the guts of isinterestingmethodentry() that extracted the Method* from the appropriate field was isolated into it's own function ConstantPoolCacheEntry::getmethod() and used by both. That would save the complicated logic of the if statements in checknooldorobsolete entries.

Karen made the following comment in her review:

For a future cleanup, do we need f2_as_vfinal_method and is_interesting_method_entry to do that as well?

And I made a somewhat lengthy reply of other stuff that I noticed, but I said I would put all that in another bug (to be fixed by someone else... Serguei? :-)).

The other thing that might make it simpler is if isvalid() is defined with PRODUCTRETURN(true); so you don't have to wrap PRODUCT around it.

Thought about that, but I went with restore metadata.h back to an untouched form. I know when to stay away from live wires... :-)

Anyway, I reviewed all three versions of the webrev and they look correct.

Thanks!

Dan

Thanks, Coleen

On 2/4/2013 4:08 PM, Daniel D. Daugherty wrote: Greetings,

I've updated the fix due to comments in Code Review Round 0. Here is a summary of changes made to the various files: src/share/vm/oops/cpCacheOop.cpp (HSX-23.6, HSX-24) src/share/vm/oops/cpCache.cpp (HSX-25) src/share/vm/oops/klassVtable.cpp - removed the new RCTRACENOCR() macro calls at Coleen's request; these files are outside of JVM/TI RedefineClasses proper so the tracing/debug style should not be dictated by JVM/TI RedefineClasses style that is currently in use - did not touch the existing RCTRACE... macros in the file; removing the existing macro calls would be outside the scope of this bug src/share/vm/oops/cpCacheOop.cpp (HSX-23.6, HSX-24) src/share/vm/oops/cpCache.cpp (HSX-25) - copy a comment from ConstantPoolCacheEntry::isinterestingmethodentry() to ConstantPoolCacheEntry::checknooldorobsoleteentries() src/share/vm/oops/klassVtable.cpp - Copied the new comment intended to prevent the "break" from re-materializing again from klassItable::adjustmethodentries() to klassVtable::adjustmethodentries(); yes, I'm paranoid. In the HSX-25 version only: src/share/vm/oops/metadata.hpp - revert changes src/share/vm/oops/cpCacheOop.cpp src/share/vm/oops/klassVtable.cpp - wrap uses of isvalid() in NOTPRODUCT macro as appropriate Here are the URLs for the updated webrevs: http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx23.6/ http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx24/ http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx25/ The JPDA stack testing that I started on Friday is still running on WinXP so I'm blocked for checking the recompile due to these changes. I'll start a recompile on Solaris X86, but that will take some time. Thanks, in advance, for more comments, suggestions or questions. Dan

On 2/1/13 12:55 PM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following JVM/TI bug: 7182152 Instrumentation hot swap test incorrect monitor count http://bugs.sun.com/bugdatabase/viewbug.do?bugid=7182152 https://jbs.oracle.com/bugs/browse/JDK-7182152 The fix for the bug in the product code is one line: src/share/vm/oops/klassVtable.cpp: @@ -992,18 +1020,50 @@ // RCTRACE macro has an embedded ResourceMark RCTRACE(0x00200000, ("itable method update: %s(%s)", newmethod->name()->asCstring(), newmethod->signature()->asCstring())); } - break; + // cannot 'break' here; see for-loop comment above. } ime++; } } } and is applicable to JDK7u10/HSX-23.6 and JDK7u14/HSX-24. Coleen already fixed the bug as part of the Perm Gen Removal (PGR) project in HSX-25. Yes, we found a 1-line bug fix buried in the monster PGR changeset. Many thanks to Coleen for her help in this bug hunt! The rest of the code in the webrevs are: - additional JVM/TI tracing code backported from Coleen's PGR changeset - additional JVM/TI tracing code added by me and forward ported to HSX-25 - a new -XX:TraceRedefineClasses=16384 flag value for finding these elusive old or obsolete methods - exposure of some printing code to the PRODUCT build so that the new tracing is available in a PRODUCT build You might be wondering why the new tracing code is exposed in a PRODUCT build. Well, it appears that more and more PRODUCT bits deployments are using JVM/TI RedefineClasses() and/or RetransformClasses() at run-time to instrument their systems. This bug (7182152) was only intermittently reproducible in the WLS environment in which it occurred so I made the tracing available in a PRODUCT build to assist in the hunt. Raj from the WLS team has also verified that the HSX-23.6 version of fix resolves the issue in his environment. Thanks Raj! Here are the URLs for the three webrevs: http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx23.6/ http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx24/ http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx25/ I have run the following test suites from the JPDA stack on the JDK7u10/HSX-23.6 version of the fix with -XX:TraceRedefineClasses=16384 specified: sdk-jdi sdk-jdiclosed sdk-jli vm-heapdump vm-hprof vm-jdb vm-jdi vm-jdwp vm-jvmti vm-sajdi The tested configs are: {Solaris-X86, WinXP} X {Client VM, Server VM} X {-Xmixed, -Xcomp} X {product, fastdebug} With the 1-liner fix in place, the new tracing code does not find any instances of this failure mode in any of the above test suites. Without the the 1-liner fix in place, the new tracing code finds one instance of this failure mode in the above test suites: test/java/lang/instrument/IsModifiableClassAgent.java There are two new tests that will be pushed to the JDK repos using a different bug ID (not yet filed): test/com/sun/jdi/RedefineAbstractClass.sh test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh There will be a separate review request for the new tests. I'm currently running the JPDA stack of tests on the JDK7u14/HSX-24 and JDK8-B75/HSX-25 versions of the fix. That testing will likely take all weekend to complete. Thanks, in advance, for any comments and/or suggestions. Dan

More information about the serviceability-dev mailing list