Thank you Dan for the really quick review!
On 9/18/2013 5:26 PM, Daniel D. Daugherty wrote:
On 9/18/13 1:37 PM, Coleen Phillimore wrote:
Hi, The new webrev is larger now. I found code where Method* can be
leaked because methodHandles are not freed, and have rewritten
JVM_GetClassDeclaredMethods and JVM_GetClassDeclaredConstructors to
be redefinition safe.
http://cr.openjdk.java.net/~coleenp/8022887_2/
<http://cr.openjdk.java.net/%7Ecoleenp/8022887_2/>
Again, your 'frames' links are broken, but those "Previous File"
and "Next File" links look interesting... :-)
Yes, one webrev version has these nice previous/next file links and the
other version of webrev gets frames correct. I like the latter better
but it's not maintained anymore. I updated the webrev with the other.
Short version: thumbs up
Longer version...
src/share/vm/runtime/handles.hpp
No comments.
src/share/vm/runtime/handles.inline.hpp
No comments.
src/share/vm/oops/instanceKlass.hpp
So this comment:
ConstantPool* _prev_constant_pool; // regular or weak
reference
I took out the comment.
is stale, i.e., leftover from the pre-PermGenRemoval days.
And it looks like you're ripping out the PreviousVersionInfo
wrapper around PreviousVersionNode stuff because the constant
pool is no longer an oop so we don't need that extra layer of
protection anymore. I definitely like the cleanup.
Thanks! It's also taken out because it held a resource allocated array
of methodHandles whose destructors were never called, so were never
deleted off the thread->metadata_handles() list, so were always marked
as live on_stack, and never deleted.
src/share/vm/oops/instanceKlass.cpp
More CDE of stuff from the pre-PermGenRemoval days. Looks good.
And buried in all the CDE, is a critical fix from the previous
round:
*** 3518,3527 ****
--- 3511,3522 ----
m = methods()->at(index);
if (m->method_idnum() == idnum) {
return m;
}
}
+ // None found, return null for the caller to handle.
+ return NULL;
}
return m;
}
src/share/vm/prims/jvm.cpp
I like the refactoring into get_class_declared_method_helper().
Can you change:
1918 return get_class_declared_method_helper(env, ofClass,
publicOnly, false,
1919 SystemDictionary::reflect_Method_klass(), THREAD);
to:
1918 return get_class_declared_method_helper(env, ofClass, publicOnly,
1919 false /*
want_constructor */,
1920 SystemDictionary::reflect_Method_klass(), THREAD);
And can you change:
1926 return get_class_declared_method_helper(env, ofClass,
publicOnly, true,
1927 SystemDictionary::reflect_Constructor_klass(), THREAD);
to:
1927 return get_class_declared_method_helper(env, ofClass, publicOnly,
1928 true /*
want_constructor */,
1929 SystemDictionary::reflect_Constructor_klass(), THREAD);
Got it. The drawback of boolean arguments.
src/share/vm/prims/jvmtiImpl.cpp
More CDE from the removal of PreviousVersionInfo. Looks good.
src/share/vm/prims/jvmtiRedefineClasses.cpp
More CDE from the removal of PreviousVersionInfo. Looks good.
So if I understand this change correctly:
1) fix Method* InstanceKlass::method_with_idnum(int idnum) {
to not return a mis-matched Method* value
2) refactor JVM_GetClassDeclaredMethods() and
JVM_GetClassDeclaredConstructors() to share common code and to
tolerate an interleaved JVM/TI RedefineClasses() call
3) CDE of PreviousVersionInfo and the cleanup of PreviousVersionWalker
that the CDE allows
Yes, but 3 was causing leaks also.
Thanks and thanks for the quick review!
Coleen
Nicely done.
Dan
Tested with internal vm.quick.testlist and mlvm tests.
Thanks,
Coleen
On 9/5/2013 2:20 PM, serguei.spit...@oracle.com wrote:
Coleen,
This is great finding, and also a nice catch by Dan.
Waiting for a new webrev from you.
Thanks,
Serguei
On 9/5/13 9:35 AM, Coleen Phillimore wrote:
Dan,
Thank you for looking at this so quickly. You are right, we are
not only getting public methods, whose number cannot change right
now with redefine classes.
I have to rework this change.
Thanks,
Coleen
On 9/5/2013 12:23 PM, Daniel D. Daugherty wrote:
On 9/5/13 9:33 AM, Coleen Phillimore wrote:
Summary: Need to refetch the methods array from InstanceKlass
after safepoint.
open webrev at http://cr.openjdk.java.net/~coleenp/8022887/
The "frames" links are broken in this webrev. I had to
write down the changed line numbers for jvm.cpp and then
use the "new" link to see the context of the changes.
src/share/vm/oops/instanceKlass.cpp
Nice catch. The old code could return an 'm' value that
referred to a method that wasn't a match. Ouch.
yes, it was a bit of a red herring for a while.
src/share/vm/prims/jvm.cpp
Nice catch of the use of potentially stale method array, but I
think there might be more issues here.
In JVM_GetClassDeclaredMethods:
line 1865: ++num_methods;
<snip>
line 1871: objArrayOop r =
oopFactory::new_objArray(SystemDictionary::reflect_Method_klass(),
num_methods, CHECK_NULL);
<snip>
line 1876: methods = k->methods();
line 1877: methods_length = methods->length();
<snip>
line 1885: result->obj_at_put(out_idx, m);
<snip>
line 1890: assert(out_idx == num_methods, "just checking");
So num_methods is computed before the new_objArray() call
that
can result in a safepoint which can permit a
RedefineClasses()
operation to complete. You refresh methods and
methods_length,
but num_methods still has its pre-RedefineClasses value and
the size of the result array is also at the
pre-RedefineClasses
size. Isn't it possible that we could overflow the result
array
here? And maybe fire that assert() on line 1890.
In JVM_GetClassDeclaredConstructors(), similar concerns for these
lines:
line 1922: ++num_constructors;
<snip>
line 1928: objArrayOop r =
oopFactory::new_objArray(SystemDictionary::reflect_Constructor_klass(),
num_constructors, CHECK_NULL);
<snip>
line 1942: result->obj_at_put(out_idx, m);
<snip>
line 1947: assert(out_idx == num_constructors, "just checking");
Yes, this RedefineClasses() stuff is a serious pain in the butt
because it can change your assumed invariants in the middle of
your function.
Dan
bug link at http://bugs.sun.com/view_bug.do?bug_id=8022887
Tested with the test cases in the bug, and with internal SQE
tests (nsk.quick.testlist).
thanks,
Coleen
