code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Dec 22 09:12:54 PST 2011
On 12/22/11 9:48 AM, Kelly O'Hair wrote:
> Just a short non critical observation, what happens when numDefs==0?
Boom! Note comment on line 1157.
1156 /*
1157 * Java code must not call this with a null list or a zero-length
list.
1158 */
1159 void
1160 redefineClasses(JNIEnv * jnienv, JPLISAgent * agent, jobjectArray
classDefinitions) {
1161 jvmtiEnv* jvmtienv = jvmti(agent);
1162 jboolean errorOccurred = JNI_FALSE;
1163 jclass classDefClass = NULL;
1164 jmethodID getDefinitionClassMethodID = NULL;
1165 jmethodID getDefinitionClassFileMethodID = NULL;
1166 jvmtiClassDefinition* classDefs = NULL;
1167 jbyteArray* targetFiles = NULL;
1168 jsize numDefs = 0;
1169
1170 jplis_assert(classDefinitions != NULL);
1171
1172 numDefs = (*jnienv)->GetArrayLength(jnienv, classDefinitions);
1173 errorOccurred = checkForThrowable(jnienv);
1174 jplis_assert(!errorOccurred);
1175
1176 if (!errorOccurred) {
1177 jplis_assert(numDefs > 0);
1178 /* get method IDs for methods to call on class definitions */
1179 classDefClass = (*jnienv)->FindClass(jnienv,
"java/lang/instrument/ClassDefinition");
1180 errorOccurred = checkForThrowable(jnienv);
1181 jplis_assert(!errorOccurred);
1182 }
For some reason, jplis_assert() is enabled in product bits
so line 1170 will fire with a NULL classDefinitions and with
a numDefs == 0, line 1177 will fire.
However, all that is on the C-code side...
The Java side is more polite:
src/share/classes/sun/instrument/InstrumentationImpl.java
132 public void
133 redefineClasses(ClassDefinition[] definitions)
134 throws ClassNotFoundException {
135 if (!isRedefineClassesSupported()) {
136 throw new
UnsupportedOperationException("redefineClasses is not supported in this
environment");
137 }
138 if (definitions == null) {
139 throw new NullPointerException("null passed as
'definitions' in redefineClasses");
140 }
141 for (int i = 0; i < definitions.length; ++i) {
142 if (definitions[i] == null) {
143 throw new NullPointerException("element of
'definitions' is null in redefineClasses");
144 }
145 }
146 if (definitions.length == 0) {
147 return; // short-circuit if there are no changes
requested
148 }
149
150 redefineClasses0(mNativeAgent, definitions);
151 }
If you pass in a null definitions array, you get a nice NPE.
If the definitions array is empty, then we don't call into
the native side...
Dan
>
> -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 <http://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/b9b643c6/attachment-0001.html
More information about the serviceability-dev
mailing list