JVM/TI code review request (XS and M) (7182152)
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::check_no_old_or_obsolete_entries 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
> is_interesting_method_entry copied to check_no_old_or_obsolete_entries
> 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 is_valid 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 f2_as_vfinal_method and is_interesting_method_entry
> 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/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
>>
More information about the serviceability-dev
mailing list