code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)
Poonam Bajaj
poonam.bajaj at oracle.com
Wed Dec 21 19:05:00 PST 2011
Hi Dan,
In the following block (in JPLISAgent.c):
/1278 if (!errorOccurred) {
1279 jvmtiError errorCode = JVMTI_ERROR_NONE;
1280 errorCode =
(*jvmtienv)->RedefineClasses(jvmtienv, numDefs, classDefs);
1281 if (errorCode == JVMTI_ERROR_WRONG_PHASE) {
1282 /* insulate caller from the wrong phase
error */
1283 errorCode = JVMTI_ERROR_NONE;
1284 } else {
1285 errorOccurred = (errorCode !=
JVMTI_ERROR_NONE);
1286 if ( errorOccurred ) {
1287
createAndThrowThrowableFromJVMTIErrorCode(jnienv, errorCode);
1288 }
1289 }
1290 }/
If RedefineClasses() fails here then
createAndThrowThrowableFromJVMTIErrorCode() is being called. At the
point we would have filled data (upto the value of index 'i') into
classDefs and targetFiles but in case of RedefineClasses failure, those
will not get freed. Is that correct ?
Thanks,
Poonam
On 12/22/2011 4:46 AM, Daniel D. Daugherty wrote:
> 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
>>>>>
--
Best regards, Poonam
Oracle <http://www.oracle.com>
Poonam Bajaj | Principal Member of Technical Staff
Phone: +91 80 66937451 <tel:+91%2080%2066937451> | Mobile: +91
9844511366 <tel:+91%209844511366>
Oracle JVM Sustaining Engineering
ORACLE India | 560102 Bangalore
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20111222/2600ab44/attachment-0001.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oracle_sig_logo.gif
Type: image/gif
Size: 658 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20111222/2600ab44/oracle_sig_logo-0001.gif
-------------- next part --------------
A non-text attachment was scrubbed...
Name: green-for-email-sig_0.gif
Type: image/gif
Size: 356 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20111222/2600ab44/green-for-email-sig_0-0001.gif
More information about the serviceability-dev
mailing list