RFR 8022887: Assertion hit while using class and redefining it with RedefineClasses s,imultaneously
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Sep 18 15:21:11 PDT 2013
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?
>
>> 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.
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 hotspot-runtime-dev
mailing list