JVM/TI code review request (XS and M) (7182152)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Feb 6 12:25:27 PST 2013
Thanks Serguei!
Dan
On 2/6/13 1:22 PM, serguei.spitsyn at oracle.com wrote:
> Looks good.
>
> Thanks,
> Serguei
>
> On 2/6/13 11:54 AM, Karen Kinnear wrote:
>> Thank you Dan - this is much better and sets a good model for the
>> rest of us.
>>
>> thanks,
>> Karen
>>
>> On Feb 6, 2013, at 9:05 AM, Daniel D. Daugherty wrote:
>>
>>> Adding other alias and people back onto the thread...
>>>
>>> Thanks for the re-review!
>>>
>>>
>>> On 2/6/13 6:41 AM, Coleen Phillimore wrote:
>>>> This is good that you added the INCLUDE_JVMTI. I didn't think it'd
>>>> add that much space, but it a good change.
>>> I didn't think it would add much space either, but...
>>>
>>> It gave me a chance to check out the MinimalVM stuff a bit and
>>> it serves to identify those pieces of code as being associated
>>> with JVM/TI.
>>>
>>> Karen and Serguei, when you get the chance, please re-review.
>>>
>>> Again, thanks for the re-review!
>>>
>>> Dan
>>>
>>>
>>>> Thumbs up!
>>>> Coleen
>>>>
>>>> On 2/6/2013 12:59 AM, Daniel D. Daugherty wrote:
>>>>> Greetings,
>>>>>
>>>>> The JPDA stack testing finished with no new regressions on HSX-23.6,
>>>>> HSX-24 or HSX-25. The HSX-24 version of the fix has been pushed.
>>>>>
>>>>> I've updated the HSX-25 version of the fix due to Karen's comments
>>>>> about the MinimalVM configuration in Code Review Round 1. In this
>>>>> latest round for HSX-25, I've made use of the INCLUDE_JVMTI define
>>>>> to limit the exposure of new tracing code to configurations that
>>>>> include JVM/TI support. The MinimalVM does not support JVM/TI so
>>>>> none of the new code needs to be included there. While I was at it,
>>>>> I also excluded some other JVM/TI RedefineClasses() support code
>>>>> from the MinimalVM config that I hadn't previously touched.
>>>>>
>>>>> In short, none of the functionality has been changed in this round.
>>>>> Just the way it is built or not built has been changed.
>>>>>
>>>>> Here is the URL for the latest HSX-25 webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~dcubed/7182152-webrev/2-hsx25/
>>>>>
>>>>> Thanks, in advance, for more comments, suggestions or questions.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 2/4/13 2:08 PM, Daniel D. Daugherty wrote:
>>>>>> Greetings,
>>>>>>
>>>>>> I've updated the fix due to comments in Code Review Round 0.
>>>>>>
>>>>>> Here is a summary of changes made to the various files:
>>>>>>
>>>>>> src/share/vm/oops/cpCacheOop.cpp (HSX-23.6, HSX-24)
>>>>>> src/share/vm/oops/cpCache.cpp (HSX-25)
>>>>>> src/share/vm/oops/klassVtable.cpp
>>>>>> - removed the new RC_TRACE_NO_CR() macro calls at Coleen's
>>>>>> request;
>>>>>> these files are outside of JVM/TI RedefineClasses proper so the
>>>>>> tracing/debug style should not be dictated by JVM/TI
>>>>>> RedefineClasses
>>>>>> style that is currently in use
>>>>>> - did not touch the existing RC_TRACE... macros in the file;
>>>>>> removing
>>>>>> the existing macro calls would be outside the scope of this bug
>>>>>>
>>>>>> src/share/vm/oops/cpCacheOop.cpp (HSX-23.6, HSX-24)
>>>>>> src/share/vm/oops/cpCache.cpp (HSX-25)
>>>>>> - copy a comment from
>>>>>> ConstantPoolCacheEntry::is_interesting_method_entry()
>>>>>> to ConstantPoolCacheEntry::check_no_old_or_obsolete_entries()
>>>>>>
>>>>>> src/share/vm/oops/klassVtable.cpp
>>>>>> - Copied the new comment intended to prevent the "break" from
>>>>>> re-materializing again from klassItable::adjust_method_entries()
>>>>>> to klassVtable::adjust_method_entries(); yes, I'm paranoid.
>>>>>>
>>>>>> In the HSX-25 version only:
>>>>>> src/share/vm/oops/metadata.hpp
>>>>>> - revert changes
>>>>>>
>>>>>> src/share/vm/oops/cpCacheOop.cpp
>>>>>> src/share/vm/oops/klassVtable.cpp
>>>>>> - wrap uses of is_valid() in NOT_PRODUCT macro as appropriate
>>>>>>
>>>>>> Here are the URLs for the updated webrevs:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx23.6/
>>>>>> http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx24/
>>>>>> http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx25/
>>>>>>
>>>>>> The JPDA stack testing that I started on Friday is still running on
>>>>>> WinXP so I'm blocked for checking the recompile due to these
>>>>>> changes.
>>>>>> I'll start a recompile on Solaris X86, but that will take some time.
>>>>>>
>>>>>> Thanks, in advance, for more comments, suggestions or questions.
>>>>>>
>>>>>> 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/
>>>>>>>
>>>>>>> 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