JVM/TI code review request (XS and M) (7182152)
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Feb 4 09:16:21 PST 2013
On 2/4/2013 11:50 AM, Daniel D. Daugherty wrote:
> 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.
Thanks, Dan. I do. People are actively trying to reduce metadata right
now, so having this additional word will cause a regression that will be
noticed for a special case. is_valid() is a bit of a hack too. It
relies on metadata being returned and given a non-zero bit pattern which
is only enabled in debug (iirc).
There's also a f1_as_method and f2_as_vfinal_method calls that return
Method* which might make the logic of your if statements simpler in
check_no_old_or_obsolete_entries().
Thanks,
Coleen
>
> 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/3fd962c5/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list