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

[serguei.spit...@oracle.com]

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. 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)) Thanks, Serguei On 2/20/19 07:10, coleen.phillim...@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