code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)
Poonam Bajaj
poonam.bajaj at oracle.com
Wed Dec 21 20:42:13 PST 2011
Hi Dan,
On 12/22/2011 8:52 AM, Daniel D. Daugherty wrote:
> On 12/21/11 8:05 PM, Poonam Bajaj wrote:
>> 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 ?
>
> No that is not correct and it fooled me also!
>
> The createAndThrowThrowableFromJVMTIErrorCode() call
> does create and throw a throwable Java object, but the
> redefineClasses() function does not stop executing at
> that point. It finishes what it was doing, freeing
> memory that needs to be freed before returning at the
> end of the function. Darn C-code doesn't behave like
> Java code. :-)
>
Thanks for clarification!
regards,
Poonam
> Dan
>
>
>>
>>
>> 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
>>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20111222/d2d2563d/attachment-0001.html
More information about the serviceability-dev
mailing list