code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)
Poonam Bajaj
poonam.bajaj at oracle.com
Wed Dec 21 20:40:45 PST 2011
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/20111222/428d0475/attachment-0001.html
More information about the serviceability-dev
mailing list