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

[Karen Kinnear]

New versions look great. Thank you.

For jdk8 only - is redefineclasses in the MINIMALVM? I missed your reply.
If so, I was assuming that the additional tracing would also be conditional
in the builds. I believe in addition to ifdef's there is a list of files not included
in MINIMALVM, but I didn't take the time to look this all up.

thanks,
Karen


On Feb 4, 2013, at 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 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
>> 
>> 
>> 
>