RFR 8022887: Assertion hit while using class and redefining it with RedefineClasses s,imultaneously
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Sep 18 15:13:06 PDT 2013
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.spitsyn at 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
>>>>>
>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list