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

[coleen.phillimore at oracle.com]

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/20190220/5839f51f/attachment-0001.html>