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

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Feb 4 08:50:37 PST 2013

Thanks for the additional comment. Reply below.

On 2/4/13 8:34 AM, Coleen Phillimore wrote:

On 2/4/2013 10:15 AM, Daniel D. Daugherty wrote:

Adding back the missing aliases and people... Sorry, I meant to send this re-all. I missed something major in my earlier review. The metadata->isvalid() flag should only be enabled in debug mode. It adds a word to all metadata.

Yes, I'm aware that it adds a word to all meta data. My change to src/share/vm/oops/metadata.hpp was intentional. Personally, I would prefer that is_valid() remain in the product bits for at least one release cycle (JDK8). I'm a fan of sanity checks in product bits so I actually wouldn't mind if it stay there permanently.

However, if you insist, I'll backout the change to src/share/vm/oops/metadata.hpp and redo the RedefineClasses() sanity check code to be appropriately #ifdef'ed.

Dan

Coleen, Thanks for the review. Replies embedded below. On 2/4/13 7:26 AM, Coleen Phillimore wrote: On 2/1/2013 6:55 PM, Daniel D. Daugherty wrote: And here is the webrev for the new tests (relative to JDK8-T&L):

http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/ As always, comments and suggestions are welcome. Dan

On 2/1/13 4:39 PM, Daniel D. Daugherty wrote: > There are two new tests that will be pushed to the JDK repos using > a different bug ID (not yet filed): New bug is now filed: 8007420 add test for 6805864 to com/sun/jdi, add test for 7182152 to java/lang/instrument http://bugs.sun.com/bugdatabase/viewbug.do?bugid=8007420 https://jbs.oracle.com/bugs/browse/JDK-8007420 Of course, the tests cannot be pushed until the HSX changes have made it into a promoted build and thus available to JDK8-T&L. 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/ In cpCache.cpp: RCTRACENOCR(0x00004000, ("")); Can this "searchable prefix" be defined in jvmtiTracing with the rest of the RCTRACE macros as some descriptive RCTRACE name? Doesn't have to be long but this is distracting stuff. Also, if you change this searchable prefix, you'd only have to change it once. In the cpCache.cpp case, the RCTRACENOCR(0x00004000, ("")) macro calls adds a searchable prefix for each dump line when the dump code is called from JVM/TI RedefineClasses tracing. Other code that calls the cpCache dump code won't get the JVM/TI RedefineClasses tracing prefix. The purpose for the prefix is so that all tracing output for a particular tracing level, e.g., 0x00004000, is grep'able together. This function void ConstantPoolCacheEntry::print(outputStream* st, int index) const { } Is general purpose code. It shouldn't have all these lines specific to redefine classes. If it needs to, the noise should be minimized. You can add a define in that trace macros.hpp file: #define RCPREFIX This code shouldn't be present in the general purpose metadata printing. The dumpvtable and dumpitable functions are arguable since they seem to only be used by redefine classes now, but I can see a general purpose use for these too that shouldn't include this noise. It wouldn't be appropriate to add the "searchable prefix" to the jvmtiTraceRedefineClasses.hpp file. The HEX values in JVM/TI RedefineClasses tracing are associated with the code being traced. In jvmtiRedefineClasses.cpp why can't dumpmethods just print the methods without these RCTRACE macros? Debug output style for JVM/TI RedefineClasses()is supposed to be done using the jvmtiTraceRedefineClasses mechanism. It was inconsistent for dumpmethods() to do its output the way it was so I fixed it. And some is printed and some is not? No, in this case, every line will have a jvmtiTraceRedefineClasses prefix on it when that tracing level is turned on. Example: RedefineClasses-0x4000: oldmethods -- RedefineClasses-0x4000: 0 ( -2) public {old} -- virtual void RedefineSubclassWithTwoInterfacesTarget.() RedefineClasses-0x4000: 1 ( 5) public {old} {obsolete} -- virtual jobject RedefineSubclassWithTwoInterfacesTarget.echo(jobject) Again, the prefix, "RedefineClasses-0x4000:", exists so that everything at that tracing level is grep'able together. This is really hard to read. The indentation came out strange in the webrev too. Yes, the original dumpmethods() code didn't follow hotspot style and I reformatted it since I was changing much of the code in the function. I tend to use "frames" view in webrevs and I don't see any issues with indentation. I see what happened with dumpmethods. I think it was refactored (to cut/paste places) and the indentation wasn't fixed. You can leave the noise in there since it's in redefine/classes, but I really object to it in the general purpose code. Coleen It looks like the call to dumpmethods() is covered by one of these RCTRACE macros. Yes, I did that intentionally. If I didn't then I would have to have redone all the print code to match jvmtiTraceRedefineClasses style. It was easier to add RCTRACENOCR() calls where needed and protect the initial call with an RCTRACEENABLED check. Why isn't that enough? Because all RedefineClasses debug output is supposed to have a prefix like this one: RedefineClasses-0x4000: oldmethods -- This is really distracting because I keep wondering why it's RCTRACENOCR (don't file a CR??) rather than reading the code. Oh, it's no carriage return. Ugh. You mean like printcr()? :-) I still would like dumpmethods to always dump all the methods so if you're debugging this you can temporarily paste this call various places without trying to figure out which RCTRACE number to give it. Sorry, that's not how debugging in RedefineClasses is supposed to work. dumpmethods() was an outlier that needed to be fixed so that it matched the rest of the jvmtiTraceRedefineClasses infrastructure. Of course, the Serviceability team is welcome to change this debugging code to suite their own style and tastes. I think that would be an easier task if it was all "the same". Dan Coleen 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

-------------- next part -------------- An HTML attachment was scrubbed... URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130204/77db5f12/attachment-0001.html

More information about the serviceability-dev mailing list