RFR 8022887: Assertion hit while using class and redefining it with RedefineClasses s,imultaneously
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Sep 19 08:06:42 PDT 2013
On 09/18/2013 06:21 PM, Daniel D. Daugherty wrote:
> On 9/18/13 4:13 PM, Coleen Phillimore wrote:
>>
>> Thank you Dan for the really quick review!
>
> No problem. I figured you wanted a quick review with JavaOne approaching
> and all...
>
>
>>
>> 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.
>
> I wonder if it is possible to merge the two... The one with the
> links was Tom R's right?
The former (I meant former above) is Tom R's version but frames break
periodically, and it's a long script to debug. I thought the plan was
to merge the previous/next file functionality but I guess it never happened.
>
>
>>
>>> 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.
>
> Right. I could have been more clear.
>
>
>>
>>>
>>> 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.
>
> Right. In the pre-PermGenRemoval code, we keyed off the weak reference
> to the constant pool. When the constant pool weak ref was no longer
> valid, then we knew that the methodHandles were no longer valid and
> they were cleaned up. So this leak was introduced as part of the
> transition of the pre-PermGenRemoval code to the new world order.
> Your fix here completes (and cleans up) that transition in this area.
>
> Very nice work.
Thank you so much!
Coleen
>
>
> Dan
>
>
>>
>>> 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