code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)
Karen Kinnear
karen.kinnear at oracle.com
Wed Dec 21 15:07:43 PST 2011
Dan,
Thank you for the quick fix - I echo Coleen's sentiments of wondering how
you managed to find the problem(s).
On Dec 21, 2011, at 5:41 PM, Daniel D. Daugherty wrote:
> 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 :-)
Yes - go for it. Given the holiday timing and how critical this fix
is - you have reviews from Coleen and 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...
Thanks - I prefer that solution.
>
> 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...
I wondered about that, but it seemed consistent with existing
usage - so I agree.
>
> Thanks again for the review.
>
> Dan
thanks,
Karen
>
>
>
>
>> 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
>>>
More information about the serviceability-dev
mailing list