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

[Daniel D. Daugherty]

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
>>
>>
>>
>
>
>