code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Dec 21 20:47:28 PST 2011
Thanks for reviewing both!
I haven't thought of any good tracing additions to the
ClassFileLoadHook code path... but when I do...
Dan
On 12/21/11 9:40 PM, Poonam Bajaj wrote:
> Hi Dan,
>
> I have looked at both the JDK and the Hotspot changes, so you can add
> me as reviewer for both.
>
> I especially like the class_load_kind addition to the following trace.
>
> 856 RC_TRACE_WITH_THREAD(0x00000001, THREAD,
> 857 ("loading name=%s kind=%d (avail_mem=" UINT64_FORMAT "K)",
> 858 the_class->external_name(), _class_load_kind,
> 859 os::available_memory() >> 10));
>
> Thanks,
> Poonam
>
> On 12/22/2011 8:53 AM, Daniel D. Daugherty wrote:
>> Poonam,
>>
>> I forgot to ask whether to put you down as a reviewer for both
>> the JDK fix and the HSX fix... so JDK only or both JDK and HSX?
>>
>> Dan
>>
>>
>> On 12/21/11 8:22 PM, 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. :-)
>>>
>>> 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/20111221/ccd6ae73/attachment-0001.html
More information about the serviceability-dev
mailing list