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

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

Karen,

Thanks for the reviews! Replies embedded below.

On 2/4/13 8:19 AM, Karen Kinnear wrote:

Dan,

All 3 versions of the code looks good. Thank you for enabling the printing for product since this type of problem is so hard to duplicate.

You're welcome.

A small note, I think it would have been easier for the internal code logic for the CPCE::checknooldorobsoleteentries to reverse the true/false, but no need to change.

The original code added by Robert on 2006.04.21 used "check_no_old_entries" so I followed his lead in my rename.

I would appreciate the comment from isinterestingmethodentry copied to checknooldorobsoleteentries about virtual and final that f2 contains a method ptr instead of a vtable index.

So this comment here on line 498:

497 if (is_vfinal()) { 498 // virtual and final so _f2 contains method ptr instead of vtable index 499 m = (methodOop)_f2;

Copied to this new block here:

475 if (is_vfinal()) { 476 methodOop m = (methodOop)_f2;

between line 475 and 476 (for the HSX23.6 version). Between line 580 and 581 in the HSX-24 version and between line 465 and 466 in the HSX-25 version.

I'll make that change.

In the jdk8 version in cpCache.cpp you've added the isvalid checks for metadata.

The is_valid() check may go away since the field it is using adds to memory footprint. Stay tuned.

For a future cleanup, do we need f2asvfinalmethod and isinterestingmethodentry to do that as well?

This line in the HSX-25 version:

466 Metadata* f2 = (Metadata*)_f2;

should probably have used f2_as_vfinal_method(). I'll have to check with Coleen to find out if there is a reason why it doesn't; there may be a good reason.

I noticed that there is quite a bit of "type cleanup" in the HSX-24 version of is_interesting_method_entry(), e.g.:

In HSX23.6:

497 if (is_vfinal()) { 498 // virtual and final so _f2 contains method ptr instead of vtable index 499 m = (methodOop)_f2; 500 } else if ((oop)_f1 == NULL) { 501 // NULL _f1 means this is a virtual entry so also not interesting 502 return false;

In HSX24:

602 if (is_vfinal()) { 603 // virtual and final so _f2 contains method ptr instead of vtable index 604 m = f2_as_vfinal_method(); 605 } else if (is_f1_null()) { 606 // NULL _f1 means this is a virtual entry so also not interesting 607 return false;

check_no_old_or_obsolete_entries() could definitely benefit from the better code in is_interesting_method_entry(). In particular, this block in is_interesting_method_entry():

491 if (!is_method_entry()) { 492 // not a method entry so not interesting by default 493 return false; 494 }

is present in the HSX-23.6, HSX-24, and HSX-25 versions of is_interesting_method_entry(), but is not used by the original check_no_old_entries() or any of the new check_no_old_or_obsolete_entries(). I'll put that info in the new bug to track the future work.

Is redefineclasses supported in the MinimalVM?

I don't know the answer to that. I have not been tracking the MinimalVM work. I'll investigate and get back to you.

Again, thanks for the reviews.

Dan

thanks, Karen On Feb 1, 2013, at 2: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