JVM/TI code review request (XS and M) (7182152)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Feb 4 08:50:37 PST 2013
Thanks for the additional comment. Reply below.
On 2/4/13 8:34 AM, Coleen Phillimore wrote:
> 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.
Yes, I'm aware that it adds a word to all meta data. My change to
src/share/vm/oops/metadata.hpp was intentional. Personally, I would
prefer that is_valid() remain in the product bits for at least one
release cycle (JDK8). I'm a fan of sanity checks in product bits so
I actually wouldn't mind if it stay there permanently.
However, if you insist, I'll backout the change to
src/share/vm/oops/metadata.hpp and redo the RedefineClasses()
sanity check code to be appropriately #ifdef'ed.
Dan
>>
>>
>> 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/77db5f12/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list