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
