code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Dec 21 15:16:08 PST 2011
On 12/21/11 4:07 PM, Karen Kinnear wrote:
> Dan,
>
> Thank you for the quick fix -
You're welcome. I'm trying to get this off my plate before
I leave for the holidays... so not exactly altruistic... :-)
> I echo Coleen's sentiments of wondering how
> you managed to find the problem(s).
I'll send a reply on that topic later...
> 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.
I have two HSX reviews (yeah!) and I have one JDK review.
I need one more JDK review...
>>
>>> 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.
I figured you might...
>> 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.
Glad we're on the same page!
Dan
>> 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