JVM/TI code review request (XS and M) (7182152)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Feb 4 13:22:44 PST 2013
Thanks for the additional test comments. Replies embedded below.
On 2/4/13 1:06 PM, Coleen Phillimore wrote:
> On 2/4/2013 12:19 PM, Daniel D. Daugherty wrote:
>> Adding back dropped aliases and people...
>
> I think I have all the aliases on this, this time.
And the individual people also. Thanks!
>>
>> Thanks for reviewing the first test! Replies embedded below.
>>
>
> Ok, now I see about the scaffolding framework for the first test. I
> can say it looks consistent with the other tests in that directory.
>
> I reviewed the second test too. I have some questions.
>
> in
> http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh.html
>
> In lines 70-77 you move the same two files twice.
I don't think so. Getting rid of the
"RedefineSubclassWithTwoInterfaces" prefix will make the
real script code more apparent:
70 mv X_Target.java \
71 X_Target_1.java
72 mv X_Target.class \
73 X_Target_1.class
74 mv X_Impl.java \
75 X_Impl_1.java
76 mv X_Impl.class \
77 X_Impl_1.class
> Also the file,
>
> http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/java/lang/instrument/RedefineSubclassWithTwoInterfacesImpl_1.java.html
>
>
> Is exactly the same as the file
> RedefineSubclassWithTwoInterfacesImpl.java
>
> Isn't one supposed to be derived from Target_1 rather than Target or
> different in some way?
The "_1" stuffis just for version naming purposes. The actual
class name has to be the same between Foo.java and Foo_1.java.
In this particular bug's case, we only needed to redefine
RedefineSubclassWithTwoInterfacesImpl with an EMCP version
to tickle the bug. That's why there are no differences
between RedefineSubclassWithTwoInterfacesImpl.java and
RedefineSubclassWithTwoInterfacesImpl_1.java.
Good eyes though.
> This second test makes more sense to me than the first anyway. Are you
> going to wait for the fix to get propagated before checking it in?
Yes, I have to wait until the HSX fix has made it to the T&L repo
before I can push the tests. Integrating failing tests intentionally
is not permitted in T&L. Of course, you can integrate them disabled,
but then you have to go back and enable the test sometime.
Dan
> Thanks,
> Coleen
>
>
>>
>> On 2/4/13 8:09 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/
>>>
>>> I am so impressed that you were able to write a test for this.
>>
>> Thanks. The shame of it is that I wrote this test back when I fixed
>> the 1-liner bug back in 2009. The test never got pushed to the JDK
>> repo so the regression was able to sneak in later. Sigh.
>>
>>
>>> I can't follow this shell script at all. Who calls createJavaFile?
>>
>> This test is written using the JDI ShellScaffold.sh infrastructure
>> in test/com/sun/jdi/ShellScaffold.sh (1106 lines of pure joy!).
>> createJavaFile() is a hook called by ShellScaffold.sh to create
>> the .java file that will be exercised by the test.
>>
>>
>>> http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/test/com/sun/jdi/RedefineAbstractClass.sh.html
>>>
>>>
>>> I think a limited amount of duplication of Java code in the test
>>> directory is worth having these scripts be more simple. It looks
>>> like the only thing that substituted in the java test is the main
>>> class, but lines 66 to 104 can be a stand-alone java source file.
>>
>> No, the main class just has the breakpoints.
>>
>> This is the line for the redefinition:
>>
>> 76 // @1 uncomment System.out.println("This is call " + counter
>> + " to checkFunc");
>>
>> so the original version doesn't have this line and new version
>> has this line. The glue that makes all that work is in TestScaffold.sh.
>>
>> I don't remember for sure, but I think JDI ShellScaffold.sh does not
>> work well with multiple .java files. It has been a long time since
>> I've written a test in this format. These JDI .sh tests are meant to
>> be standalone if need be. That's why this comment is always here:
>>
>> 143 # You could replace this next line with the contents
>> 144 # of ShellScaffold.sh and this script will run just the same.
>> 145 mysetup
>>
>>
>>> Again, I can't really follow the logic here and I dread ever having
>>> to debug this. Can you simplify this?
>>
>> Which part? By definition, RedefineClasses tests are complicated because
>> you have to have at least two versions of .class files, sometimes more.
>> This test is reproducing a bug using "jdb" so there is all the infra
>> that supports controlling "jdb".
>>
>> I think this test is as simple as it can be given the context, but I'm
>> open to specific simplification suggestions.
>>
>> Did you plan to review the java.lang.instrument (JLI) test? It's a bit
>> more convoluted because it is trying to mimic the WLS test case.
>>
>> Dan
>>
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>>
>>>> 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/
>>>>>>
>>>>>> 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