RFR (M) 8078725: method adjustments can be done just once for all classes involved into redefinition

Thu Feb 21 14:09:32 UTC 2019

Thank you for the code review, Serguei!
Coleen

On 2/21/19 4:24 AM, serguei.spitsyn at oracle.com wrote:
> Coleen,
>
> Reviewed now.
> Thank you for the explanations!
>
> Thanks,
> Serguei
>
>
> On 2/20/19 17:53, coleen.phillimore at oracle.com wrote:
>>
>> Serguei, Thank you for reviewing!
>>
>> On 2/20/19 8:38 PM, serguei.spitsyn at oracle.com wrote:
>>> Hi Coleen,
>>>
>>> It looks great to me!
>>> The overhead does not grow quadratically anymore with the number of  
>>> redefined classes.
>>> Also, several spots in code are simplified with this change.
>>>
>>> One minor comment:
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev/src/hotspot/share/prims/jvmtiRedefineClasses.hpp.frames.html
>>> 518 // to fix up these pointers and clean MethodData out
>>>    Dot is missed at the end of comment.
>>>
>>
>> Fixed.
>>
>>>
>>> One question:
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev/src/hotspot/share/prims/jvmtiRedefineClasses.cpp.udiff.html
>>> - // Fix the vtable embedded in the_class and subclasses of the_class,
>>> - // if one exists. We discard scratch_class and we don't keep an
>>> - // InstanceKlass around to hold obsolete methods so we don't have
>>> - // any other InstanceKlass embedded vtables to update. The vtable
>>> - // holds the Method*s for virtual (but not final) methods.
>>> - // Default methods, or concrete methods in interfaces are stored
>>> - // in the vtable, so if an interface changes we need to check
>>> - // adjust_method_entries() for every InstanceKlass, which will also
>>> - // adjust the default method vtable indices.
>>> - // We also need to adjust any default method entries that are
>>> - // not yet in the vtable, because the vtable setup is in progress.
>>> - // This must be done after we adjust the default_methods and
>>> - // default_vtable_indices for methods already in the vtable.
>>> - // If redefining Unsafe, walk all the vtables looking for entries.
>>> - if (ik->vtable_length() > 0 && (_the_class->is_interface()
>>> - || _the_class == SystemDictionary::internal_Unsafe_klass()
>>> - || ik->is_subtype_of(_the_class))) {
>>> - // ik->vtable() creates a wrapper object; rm cleans it up
>>> + // Adjust all vtables, default methods and itables, to clean out 
>>> old methods.
>>>         ResourceMark rm(_thread);
>>> -
>>> - ik->vtable().adjust_method_entries(the_class, &trace_name_printed);
>>> - ik->adjust_default_methods(the_class, &trace_name_printed);
>>> + if (ik->vtable_length() > 0) {
>>> + ik->vtable().adjust_method_entries(&trace_name_printed);
>>> + ik->adjust_default_methods(&trace_name_printed);
>>>       }
>>>   
>>> - // If the current class has an itable and we are either redefining an
>>> - // interface or if the current class is a subclass of the_class, then
>>> - // we potentially have to fix the itable. If we are redefining an
>>> - // interface, then we have to call adjust_method_entries() for
>>> - // every InstanceKlass that has an itable since there isn't a
>>> - // subclass relationship between an interface and an InstanceKlass.
>>> - // If redefining Unsafe, walk all the itables looking for entries.
>>> - if (ik->itable_length() > 0 && (_the_class->is_interface()
>>> - || _the_class == SystemDictionary::internal_Unsafe_klass()
>>> - || ik->is_subclass_of(_the_class))) {
>>> - ResourceMark rm(_thread);
>>> - ik->itable().adjust_method_entries(the_class, &trace_name_printed);
>>> + if (ik->itable_length() > 0) {
>>> + ik->itable().adjust_method_entries(&trace_name_printed);
>>>       }
>>>   It is not clear what was the motivation to remove comments
>>>   and a part of conditions above:
>>> - && (_the_class->is_interface()
>>> - || _the_class == SystemDictionary::internal_Unsafe_klass()
>>> - || ik->is_subtype_of(_the_class))
>>>
>>
>> The old code could skip adjusting entries for vtables and itables 
>> depending on the class that it was redefined, whereas the new code 
>> doesn't walk the CLDG per class.  There is no _the_class anymore.  So 
>> the whole function is simplified to unconditionally go through the 
>> itables and vtables.  If we wanted to optimize further, we could add 
>> these conditions based on "are any of the classes redefined an 
>> interface, etc".  I didn't think it was worth the complication.
>>
>> Most of the comments are explaining why we may or may not have to 
>> visit the metadata.  I didn't think scavenging bits of the comments 
>> would be particularly helpful to the reader.
>>
>> Thanks,
>> Coleen
>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 2/20/19 07:10, coleen.phillimore at oracle.com wrote:
>>>> Summary: walk all classes at the end of redefinition and adjust 
>>>> method entries and clean MethodData
>>>>
>>>> This improves performance for redefining multiple classes at once. 
>>>> See CR for more information.
>>>>
>>>> Tested with redefinition tests in repository, and hs tier1-3.
>>>>
>>>> make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti 
>>>> >&jvmti.out
>>>> make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jdi >&jdi.out
>>>> make test TEST=open/test/hotspot/jtreg/runtime/RedefineTests 
>>>> >&redefine.out
>>>> make test TEST=open/test/hotspot/jtreg/runtime/logging >&logging.out
>>>> make test TEST=open/test/jdk/java/lang/instrument >&instrument.out
>>>> make test TEST=open/test/jdk/com/sun/jdi >&jtreg.jdi.out
>>>>
>>>> open webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8078725
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190221/d576b338/attachment-0001.html>