Thank you Dan - this is much better and sets a good model for the rest of us.
thanks, Karen On Feb 6, 2013, at 9:05 AM, Daniel D. Daugherty wrote: > Adding other alias and people back onto the thread... > > Thanks for the re-review! > > > On 2/6/13 6:41 AM, Coleen Phillimore wrote: >> >> This is good that you added the INCLUDE_JVMTI. I didn't think it'd add that >> much space, but it a good change. > > I didn't think it would add much space either, but... > > It gave me a chance to check out the MinimalVM stuff a bit and > it serves to identify those pieces of code as being associated > with JVM/TI. > > Karen and Serguei, when you get the chance, please re-review. > > Again, thanks for the re-review! > > Dan > > >> Thumbs up! >> Coleen >> >> On 2/6/2013 12:59 AM, Daniel D. Daugherty wrote: >>> Greetings, >>> >>> The JPDA stack testing finished with no new regressions on HSX-23.6, >>> HSX-24 or HSX-25. The HSX-24 version of the fix has been pushed. >>> >>> I've updated the HSX-25 version of the fix due to Karen's comments >>> about the MinimalVM configuration in Code Review Round 1. In this >>> latest round for HSX-25, I've made use of the INCLUDE_JVMTI define >>> to limit the exposure of new tracing code to configurations that >>> include JVM/TI support. The MinimalVM does not support JVM/TI so >>> none of the new code needs to be included there. While I was at it, >>> I also excluded some other JVM/TI RedefineClasses() support code >>> from the MinimalVM config that I hadn't previously touched. >>> >>> In short, none of the functionality has been changed in this round. >>> Just the way it is built or not built has been changed. >>> >>> Here is the URL for the latest HSX-25 webrev: >>> >>> http://cr.openjdk.java.net/~dcubed/7182152-webrev/2-hsx25/ >>> >>> Thanks, in advance, for more comments, suggestions or questions. >>> >>> Dan >>> >>> >>> On 2/4/13 2: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 RC_TRACE_NO_CR() 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 RC_TRACE... 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::is_interesting_method_entry() >>>> to ConstantPoolCacheEntry::check_no_old_or_obsolete_entries() >>>> >>>> src/share/vm/oops/klassVtable.cpp >>>> - Copied the new comment intended to prevent the "break" from >>>> re-materializing again from klassItable::adjust_method_entries() >>>> to klassVtable::adjust_method_entries(); 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 is_valid() in NOT_PRODUCT 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/view_bug.do?bug_id=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 @@ >>>>> // RC_TRACE macro has an embedded ResourceMark >>>>> RC_TRACE(0x00200000, ("itable method update: %s(%s)", >>>>> new_method->name()->as_C_string(), >>>>> new_method->signature()->as_C_string())); >>>>> } >>>>> - 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-jdi_closed >>>>> 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 >>>>> >>>>> >>>>> >>>> >>>> >>>> >>> >> >
