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 hotspot-runtime-dev mailing list