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

[Coleen Phillimore]

Sure, that's okay then.
Coleen

On 12/21/2011 4:25 PM, Daniel D. Daugherty wrote:
> Coleen,
>
> Thanks for the quick review!
>
> I don't think it would be too hard to refactor that code, but
> I would prefer not to for a couple of reasons:
>
> - the JDK6u29 version will likely be pushed as an escalation fix
>   and refactoring is frowned upon unless absolutely necessary
> - for ease of determining correctness with all three versions,
>   they should remain as similar as possible
>
> Are you OK with this resolution? Please let me know.
>
> Dan
>
>
> On 12/21/11 2:06 PM, Coleen Phillimore wrote:
>>
>> Dan, I reviewed one of the hotspot versions and it looks good to me.  
>> I have one request that the code in parseClassFile in the 
>> conditional  JvmtiExport::should_post_class_file_load_hook() { ... } 
>> be made into it's own function because it's not the straight line 
>> case and is getting to be a larger block of code.  If it's not hard 
>> to do.
>> We still want to know how you found it... very impressive!
>>
>> Thanks,
>> Coleen
>>
>> On 12/21/2011 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
>>>