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 |
- RFR (M) 8078725: method adjustments can be done... coleen . phillimore
- Re: RFR (M) 8078725: method adjustments ca... serguei.spit...@oracle.com
- Re: RFR (M) 8078725: method adjustment... coleen . phillimore
- Re: RFR (M) 8078725: method adjust... serguei.spit...@oracle.com
- Re: RFR (M) 8078725: method ad... coleen . phillimore
- Re: RFR (M) 8078725: method adjustments ca... Daniel D. Daugherty