JVM/TI code review request (XS and M) (7182152)
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Feb 4 07:34:54 PST 2013
On 2/4/2013 10:15 AM, Daniel D. Daugherty wrote:
> Adding back the missing aliases and people...
Sorry, I meant to send this re-all.
I missed something major in my earlier review.
The metadata->is_valid() flag should only be enabled in debug mode. It
adds a word to all metadata.
>
>
> Coleen,
>
> 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
>>>>> 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/
>>
>> 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.
This function
void ConstantPoolCacheEntry::print(outputStream* st, int index) const {
}
Is general purpose code. It shouldn't have all these lines specific to
redefine classes. If it needs to, the noise should be minimized. You
can add a define in that trace macros.hpp file:
#define RC_PREFIX <all that stuff>
This code shouldn't be present in the general purpose metadata
printing. The dump_vtable and dump_itable functions are arguable
since they seem to only be used by redefine classes now, but I can see a
general purpose use for these too that shouldn't include this noise.
>
> 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
> RedefineSubclassWithTwoInterfacesTarget.<init>()
> 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.
>
>
I see what happened with dump_methods. I think it was refactored (to
cut/paste places) and the indentation wasn't fixed. You can leave the
noise in there since it's in redefine/classes, but I really object to it
in the general purpose code.
Coleen
>
>> 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".
>
> Dan
>
>
>>
>> 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/c5f275aa/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list