JVM/TI code review request (XS and M) (7182152)

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Feb 4 16:19:54 PST 2013


On 2/4/13 4:17 PM, Coleen Phillimore wrote:
> Hi Dan,
>
> I think this version looks good.

Thanks!


> From your other reply:
>
> On 2/4/2013 11:40 AM, Daniel D. Daugherty wrote:
>> is present in the HSX-23.6, HSX-24, and HSX-25 versions of
>> is_interesting_method_entry(), but is not used by the original
>> check_no_old_entries() or any of the new 
>> check_no_old_or_obsolete_entries().
>> I'll put that info in the new bug to track the future work. 
>
> what is this new bug?  It would be nice if the guts of 
> is_interesting_method_entry() that extracted the Method* from the 
> appropriate field was isolated into it's own function 
> ConstantPoolCacheEntry::get_method() and used by both.   That would 
> save the complicated logic of the if statements in 
> check_no_old_or_obsolete entries.

Karen made the following comment in her review:

 > For a future cleanup, do we need f2_as_vfinal_method and
 > is_interesting_method_entry to do that as well?

And I made a somewhat lengthy reply of other stuff that I noticed,
but I said I would put all that in another bug (to be fixed by
someone else... Serguei? :-)).


>
> The other thing that might make it simpler is if is_valid() is defined 
> with PRODUCT_RETURN_(true); so you don't have to wrap PRODUCT around it.

Thought about that, but I went with restore metadata.h back to an untouched
form. I know when to stay away from live wires... :-)


>
> Anyway, I reviewed all three versions of the webrev and they look 
> correct.

Thanks!

Dan



> Thanks,
> Coleen
>
>
> On 2/4/2013 4: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