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

[Karen Kinnear]

Dan,

All versions of hotspot changes look good.

For the JDK and test side:
1) minor: JPLISAgent.c - new lines 1210-1212 might just be an } else {
2) new lines 1311-1312
    Why do you break if an error occurred? 
    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.

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
>