Just a short non critical observation, what happens when numDefs==0?
-kto
On Dec 21, 2011, at 8:47 PM, Daniel D. Daugherty wrote:
> 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
>>>>>>>>>>
>>>>>>>>>>
