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

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Feb 4 07:15:26 PST 2013

Adding back the missing aliases and people...


Thanks for the review. Replies embedded below.

On 2/4/13 7:26 AM, Coleen Phillimore wrote:
> On 2/1/2013 6:55 PM, Daniel D. Daugherty wrote:
>> And here is the webrev for the new tests (relative to JDK8-T&L):
>> http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/
>> As always, comments and suggestions are welcome.
>> Dan
>> On 2/1/13 4:39 PM, Daniel D. Daugherty wrote:
>>> > There are two new tests that will be pushed to the JDK repos using
>>> > a different bug ID (not yet filed):
>>> New bug is now filed:
>>>     8007420 add test for 6805864 to com/sun/jdi, add test for 7182152
>>>             to java/lang/instrument
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007420
>>> https://jbs.oracle.com/bugs/browse/JDK-8007420
>>> Of course, the tests cannot be pushed until the HSX changes have made
>>> it into a promoted build and thus available to JDK8-T&L.
>>> 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 
>>>> 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/
> In cpCache.cpp:
> RC_TRACE_NO_CR(0x00004000, (""));
> Can this "searchable prefix" be defined in jvmtiTracing with the rest 
> of the RC_TRACE macros as some descriptive RC_TRACE name? Doesn't have 
> to be long but this is distracting stuff.   Also, if you change this 
> searchable prefix, you'd only have to change it once.

In the cpCache.cpp case, the RC_TRACE_NO_CR(0x00004000, ("")) macro
calls adds a searchable prefix for each dump line when the dump code
is called from JVM/TI RedefineClasses tracing. Other code that calls
the cpCache dump code won't get the JVM/TI RedefineClasses tracing
prefix. The purpose for the prefix is so that all tracing output for
a particular tracing level, e.g., 0x00004000, is grep'able together.

It wouldn't be appropriate to add the "searchable prefix" to the
jvmtiTraceRedefineClasses.hpp file. The HEX values in JVM/TI
RedefineClasses tracing are associated with the code being traced.

> In jvmtiRedefineClasses.cpp why can't dump_methods just print the 
> methods without these RC_TRACE macros? 

Debug output style for JVM/TI RedefineClasses()is supposed to be done
using the jvmtiTraceRedefineClasses mechanism. It was inconsistent for
dump_methods() to do its output the way it was so I fixed it.

> And some is printed and some is not?

No, in this case, every line will have a jvmtiTraceRedefineClasses
prefix on it when that tracing level is turned on. Example:

RedefineClasses-0x4000: _old_methods --
RedefineClasses-0x4000: 0 ( -2) public {old} -- virtual void 
RedefineClasses-0x4000: 1 ( 5) public {old} {obsolete} -- virtual 
jobject RedefineSubclassWithTwoInterfacesTarget.echo(jobject)

Again, the prefix, "RedefineClasses-0x4000:", exists so that everything
at that tracing level is grep'able together.

> This is really hard to read.   The indentation came out strange in the 
> webrev too.

Yes, the original dump_methods() code didn't follow hotspot
style and I reformatted it since I was changing much of the
code in the function.

I tend to use "frames" view in webrevs and I don't see any
issues with indentation.

> It looks like the call to dump_methods() is covered by one of these 
> RC_TRACE macros.

Yes, I did that intentionally. If I didn't then I would have to have
redone all the print code to match jvmtiTraceRedefineClasses style.
It was easier to add RC_TRACE_NO_CR() calls where needed and protect
the initial call with an RC_TRACE_ENABLED check.

> Why isn't that enough?

Because all RedefineClasses debug output is supposed to have a
prefix like this one:

RedefineClasses-0x4000: _old_methods --

> This is really distracting because I keep wondering why it's 
> RC_TRACE_NO_CR (don't file a CR??) rather than reading the code. Oh, 
> it's no carriage return.   Ugh.

You mean like print_cr()? :-)

> I still would like dump_methods to always dump all the methods so if 
> you're debugging this you can temporarily paste this call various 
> places without trying to figure out which RC_TRACE number to give it.

Sorry, that's not how debugging in RedefineClasses is supposed to work.
dump_methods() was an outlier that needed to be fixed so that it matched
the rest of the jvmtiTraceRedefineClasses infrastructure.

Of course, the Serviceability team is welcome to change this debugging
code to suite their own style and tastes. I think that would be an easier
task if it was all "the same".


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130204/3331856b/attachment-0001.html 

More information about the hotspot-runtime-dev mailing list