code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)

[Daniel D. Daugherty]

Karen,

Thanks for the quick review! Replies in-lined below...


On 12/21/11 2:47 PM, Karen Kinnear wrote:
> Dan,
>
> All versions of hotspot changes look good.

Thanks! Are you OK if I don't wait for someone from the new
Serviceability team on this review? Serguei has already left
for the holidays... and I told him to ignore any e-mails from
me :-)


> For the JDK and test side:
> 1) minor: JPLISAgent.c - new lines 1210-1212 might just be an } else {

Nice catch! This line:

1212         if (!errorOccurred) {

should change back to:

1212         else {

I missed that when I was backing out some more complicated
stuff that I gave up on.

Fixed in all three versions.


> 2) new lines 1311-1312
>      Why do you break if an error occurred?

Because I was trying to match the existing style
too much. The for-loop from lines 1235-1276 does
that: if I have an error, then break...


>      If there is a reason to stop freeing memory, would that also apply if
>      you already had had an error? So you would want to check a new cleanuperrorOccurred
>      regardless of pre-existing error?
>      But I suspect leaving out the break would make more sense.

For this block:

1304                         /*
1305                          * Only check for error if we didn't 
already have one
1306                          * so we don't overwrite errorOccurred.
1307                          */
1308                         if (!errorOccurred) {
1309                             errorOccurred = checkForThrowable(jnienv);
1310                             jplis_assert(!errorOccurred);
1311                             if (errorOccurred) {
1312                                 break;
1313                             }
1314                         }

I'm going to delete lines 1311-1313 so we'll just try to keep
freeing as many of the memory arrays as we can...

Fixed in all three versions.

On a related note, jplis_assert() appears to be enabled even
in product bits. Which means if one of the many jplis_assert()
calls fails, then the agent terminates. I don't know the history
behind it being enabled in all bits, but I figure that's a
different issue for the Serviceability team to mull on...

Thanks again for the review.

Dan




> thanks,
> Karen
>
> On Dec 21, 2011, at 2:09 PM, Daniel D. Daugherty wrote:
>
>> Greetings,
>>
>> This is a code review request for "day one" memory leaks in
>> the java.lang.instrument.Instrumentation.redefineClasses()
>> and JVM/TI RetransformClasses() APIs.
>>
>> Since one leak is in the JDK and the other leak is in the
>> VM, I'm sending out separate webrevs for each repo. I'm also
>> attacking these leaks in three releases in parallel so there
>> is a pair of webrevs for: JDK6u29, JDK7u4 and JDK8. Yes, I'm
>> trying to get this all done before Christmas!
>>
>> Here are the bug links:
>>
>>     7121600 2/3 Instrumentation.redefineClasses() leaks class bytes
>>     http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121600
>>     http://monaco.sfbay.sun.com/detail.jsp?cr=7121600
>>
>>     7122253 2/3 Instrumentation.retransformClasses() leaks class bytes
>>     http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121600
>>     http://monaco.sfbay.sun.com/detail.jsp?cr=7122253
>>
>> I don't know why the bugs.sun.com links aren't showing up yet...
>>
>> I think it is best to review the JDK7u4/HSX-23-B06 webrevs first.
>>
>> Here are the webrevs for the JDK6u29/HSX-20 version of the fixes:
>>
>>     http://cr.openjdk.java.net/~dcubed/7121600-webrev/1-jdk6u29/
>>     http://cr.openjdk.java.net/~dcubed/7122253-webrev/0-hsx20/
>>
>> I expect the OpenJDK6 version of the fixes to very similar if not
>> identical to the JDK6u29 version. I haven't been tracking the
>> OpenJDK6 project as closely as I used to so I don't know whether
>> these fixes should go back there also.
>>
>> Here are the webrevs for the JDK7u4/HSX-23-B06 version of the fixes:
>>
>>     http://cr.openjdk.java.net/~dcubed/7121600-webrev/1-jdk7u4/
>>     http://cr.openjdk.java.net/~dcubed/7122253-webrev/0-hsx23-b06/
>>
>> The JDK7u4 JLI code has some other, unrelated fixes relative to
>> the JDK6u29 JLI code. That required a very minor change in my fix
>> to JPLISAgent.c to insulate against unexpected JVM/TI phase values
>> in a slightly different way than the original JDK7u4 code.
>>
>> Also, JDK7u4 was updated to the HSX-23-B08 snapshot last night, but
>> I baselined and tested the fix against HSX-23-B06 so I'm sending out
>> the webrev for what I actually used.
>>
>> Here are the webrevs for the JDK8/HSX-23-B08 version of the fixes:
>>
>>     http://cr.openjdk.java.net/~dcubed/7121600-webrev/1-jdk8/
>>     http://cr.openjdk.java.net/~dcubed/7122253-webrev/0-hsx23-b08/
>>
>> The JDK7u4 and JDK8 versions of the fix for 7121600 are identical.
>> The HSX-23-B06 and HSX-23-B08 versions of the fix for 7122253 are
>> also identical.
>>
>> I've run the following test suites:
>>
>> - VM/NSK jvmti, VM/NSK jvmti_unit
>> - VM/NSK jdwp
>> - SDK/JDK com/sun/jdi, SDK/JDK closed/com/sun/jdi, VM/NSK jdi
>> - SDK/JDK java/lang/instrument
>> - VM/NSK hprof, VM/NSK jdb, VM/NSK sajdi
>> - VM/NSK heapdump
>> - SDK/JDK misc_attach, SDK/JDK misc_jvmstat, SDK/JDK misc_tools
>>
>> on the following configs:
>>
>> - {Client VM, Server VM} x {product, fastdebug} x {-Xmixed, -Xcomp}
>> - Solaris X86 JDK6u29/HSX-20 (done - no regressions)
>> - Solaris X86 JDK7u4/HSX-23-B06 (done - no regressions)
>> - WinXP JDK7u4/HSX-23-B06 (in process, no regressions so far)
>> - Solaris X86 JDK8/HSX-23-B08 (just started)
>> - WinXP JDK8/HSX-23-B08 (not yet started)
>>
>> Thanks, in advance, for any feedback...
>>
>> Dan
>>