[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (M) 8078725: method adjustments can be done just once for all classes involved into redefini
From:       "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date:       2019-02-25 17:40:18
Message-ID: fff87af1-461a-03e5-f49c-8537efeaabf2 () oracle ! com
[Download RAW message or body]

Sorry for the delay in getting to this review... I was mostly
out of the office last week...


On 2/20/19 10:10 AM, coleen.phillimore@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

src/hotspot/share/oops/method.hpp
     Here's the newly refactored function:

     L977   Method* get_new_method() const {
     L978     InstanceKlass* holder = method_holder();
     L979     Method* new_method = 
holder->method_with_idnum(orig_method_idnum());
     L980
     L981     assert(new_method != NULL, "method_with_idnum() should not 
be NULL");
     L982     assert(this != new_method, "sanity check");
     L983     return new_method;
     L984   }

     As long as the replaced code always used old_method->method_holder(),
     we have equivalence. Update: verified that was the case.

src/hotspot/share/oops/cpCache.hpp
     No comments.

src/hotspot/share/oops/cpCache.cpp
     ConstantPoolCache::adjust_method_entries() no longer has the holder
     parameter so ConstantPoolCacheEntry::get_interesting_method_entry()
     can no longer verify that "interesting method" belongs to the
     interesting class. Covered in jvmtiRedefineClasses.cpp below.

     get_new_method() makes these sanity checks so removal is okay:
     new L789:     Method* new_method = old_method->get_new_method();
     old L794:     assert(new_method != NULL, "method_with_idnum() 
should not be NULL");
     old L795:     assert(old_method != new_method, "sanity check");

src/hotspot/share/oops/instanceKlass.hpp
     No comments.

src/hotspot/share/oops/instanceKlass.cpp
     InstanceKlass::adjust_default_methods() no longer has the holder
     parameter so it can no longer skip entries that belong to a
     different holder. Covered in jvmtiRedefineClasses.cpp below.

     get_new_method() makes these sanity checks so removal is okay:
     new L2922:       Method* new_method = old_method->get_new_method();
     old L2925:       assert(new_method != NULL, "method_with_idnum() 
should not be NULL");
     old L2926:       assert(old_method != new_method, "sanity check");

src/hotspot/share/oops/klassVtable.hpp
     No comments.

src/hotspot/share/oops/klassVtable.cpp
     klassVtable::adjust_method_entries() no longer has the holder
     parameter so it can no longer skip entries that belong to a
     different holder. Covered in jvmtiRedefineClasses.cpp below.

     get_new_method() makes these sanity checks so removal is okay:
     new L954:     Method* new_method = old_method->get_new_method();
     old L956:     assert(new_method != NULL, "method_with_idnum() 
should not be NULL");
     old L957:     assert(old_method != new_method, "sanity check");

     klassItable::adjust_method_entries() no longer has the holder
     parameter so it can no longer skip entries that belong to a
     different holder. Covered in jvmtiRedefineClasses.cpp below.

     get_new_method() makes these sanity checks so removal is okay:
     new L1281:     Method* new_method = old_method->get_new_method();
     old L1287:     assert(new_method != NULL, "method_with_idnum() 
should not be NULL");
     old L1288:     assert(old_method != new_method, "sanity check");

src/hotspot/share/prims/jvmtiRedefineClasses.hpp
     L356:   Klass*                      _the_class;
         '_the_class' is no longer static. Interesting...
         Any particular reason?

     L518:   // to fix up these pointers and clean MethodData out
         nit - missing a '.' at the end. Serguei may have flagged this.

src/hotspot/share/prims/jvmtiRedefineClasses.cpp
     L3423: // to fix up these pointers.  MethodData also points to old 
methods and
         nit - please delete extra space after '.'

     L3426: // Adjust cpools and vtables closure
     L3427: void 
VM_RedefineClasses::AdjustAndCleanMetadata::do_klass(Klass* k) {
         Comment needs adjusting to the new class name.

     So the key piece of the fix is the movement of this work from
     VM_RedefineClasses::redefine_single_class():

     old L3964: void VM_RedefineClasses::redefine_single_class(jclass 
the_jclass,
     <snip>
     old L4198:   AdjustCpoolCacheAndVtable 
adjust_cpool_cache_and_vtable(THREAD);
     old L4199: 
ClassLoaderDataGraph::classes_do(&adjust_cpool_cache_and_vtable);

     to here in VM_RedefineClasses::doit() after all the classes have 
been redefined:

     new L191: void VM_RedefineClasses::doit() {
     <snip>
     new L225:   AdjustAndCleanMetadata adjust_and_clean_metadata(thread);
     new L226: ClassLoaderDataGraph::classes_do(&adjust_and_clean_metadata);

     So the adjust_cpool_cache_and_vtable work and the 
clean_weak_method_links work
     are now handled by adjust_and_clean_metadata. Now that I grok that, 
let's make
     sure all the i's are dotted and t's are crossed...


     old L3430:   if (k->is_array_klass() && _the_class == 
SystemDictionary::Object_klass()) {
     old L3431:     k->vtable().adjust_method_entries(the_class, 
&trace_name_printed);
     new L3435:   if (k->is_array_klass() && _has_redefined_Object) {
     new L3436: k->vtable().adjust_method_entries(&trace_name_printed);
         For the old code, we call adjust_method_entries() on an array klass
         if the current class being redefined is java.lang.Object and 
the only
         interesting method entries are those that refer to java.lang.Object
         methods.

         For the new code, we call adjust_method_entries() on an array klass
         if java.lang.Object has been redefined as part of the set of 
classes
         that were redefined. Since we're now adjusting after all the 
redefines
         are done, that makes sense.

         Also, we don't pass in 'the_class' to adjust_method_entries() 
anymore
         so now more entries in k->vtable() are going to be considered 
interesting
         when we're trying to adjust the entries for just 'the_class'. 
What happens
         if 'k->vtable()' has an entry with the same method name as a 
method in
         'the_class', then we will always consider it interesting even 
if the holder
         associated with that entry is not 'the_class'. I don't think 
that is right.

         Update: In the case I mentioned in the previous paragraph, the 
relevant
         adjust_method_entries() code is this:

         old L949:     if (old_method == NULL || 
old_method->method_holder() != holder || !old_method->is_old()) {
         new L949:     if (old_method == NULL || !old_method->is_old()) {

         In the old code, if we were redefining java.lang.Object and the 
method in
         question was from a different class and had the same name as a 
method in
         j.l.Object, then we would have bailed out.

         In the new code, if we had redefined java.lang.Object as part 
of the
         set of classes and the method in question was from a different 
class
         and had the same name as a method in j.l.Object, then we would only
         bail out if old_method->is_old() == false. That makes sense to 
me since
         we would only want to do the adjustment if old_method had 
_also_ been
         redefined as part of the set of classes. OK, it took me a bit 
to reason
         it out, but I'm good with this one.

     old L3449:     bool is_user_defined = (_the_class->class_loader() 
!= NULL);
     old L3450:     if (is_user_defined && ik->class_loader() == NULL) {
     new L3464:     if (!_has_null_class_loader && ik->class_loader() == 
NULL) {
         I'm having trouble with the equivalence of these two pieces of code
         so let me write it out in English:

         The old code says: if the class I'm currently redefining has a 
class
         loader and the current class does not have a class loader, then the
         current class is not interesting so skip it.

         The new code says: if none of the classes we redefined in the set
         have a NULL class loader and the current class has a NULL class
         loader, then the current class is not interesting so skip it.

         Okay, that makes sense now.

     old L3472:       ResourceMark rm(_thread);
     old L3488:       ResourceMark rm(_thread);
     new L3469:     ResourceMark rm(_thread);
         In the old code, the 'rm' was only created if we knew that
         ik->vtable() or ik->itable() was going to be called. Now
         you are always creating the 'rm'. Since there's a thread
         parameter, that 'rm' is not that costly, but it is a change.

     old L3468:     if (ik->vtable_length() > 0 && 
(_the_class->is_interface()
     old L3469:         || _the_class == 
SystemDictionary::internal_Unsafe_klass()
     old L3470:         || ik->is_subtype_of(_the_class))) {
     new L3470:     if (ik->vtable_length() > 0) {
         Because of the adjustment move from redefine_single_class() to
         VM_RedefineClasses::doit(), we're losing these checks that would
         allow us to skip work on the current 'ik'. It's possible to add
         flags for:
           - was an interface redefined?
           - was SystemDictionary::internal_Unsafe_klass() redefined?
         but would it be worth it? I don't see a way to keep the
         is_subtype_of() optimization.

     old L3474: ik->vtable().adjust_method_entries(the_class, 
&trace_name_printed);
     old L3475:       ik->adjust_default_methods(the_class, 
&trace_name_printed);
     new L3471: ik->vtable().adjust_method_entries(&trace_name_printed);
     new L3472: ik->adjust_default_methods(&trace_name_printed);
         Again, we don't pass in 'the_class' to adjust_method_entries() 
anymore so
         an entry in ik->vtable() will be "interesting" only if it has 
been redefined.

         Similarly, adjust_default_methods() is no longer passed 
'the_class' so
         a default method is only "interesting" if it has been redefined.

     old L3485:     if (ik->itable_length() > 0 && 
(_the_class->is_interface()
     old L3486:         || _the_class == 
SystemDictionary::internal_Unsafe_klass()
     old L3487:         || ik->is_subclass_of(_the_class))) {
     new L3475:     if (ik->itable_length() > 0) {
         Same comments as for the vtable_length() expression change above.

     old L3489: ik->itable().adjust_method_entries(the_class, 
&trace_name_printed);
     new L3476: ik->itable().adjust_method_entries(&trace_name_printed);
         Same comments as for the vtable adjust_method_entries() change 
above.

     old L3513: cp_cache->adjust_method_entries(the_class, 
&trace_name_printed);
     new L3500: cp_cache->adjust_method_entries(&trace_name_printed);
     old L3523:         cp_cache->adjust_method_entries(pv_node, 
&trace_name_printed);
     new L3510: cp_cache->adjust_method_entries(&trace_name_printed);
         Same comments as for the vtable adjust_method_entries() change 
above.

src/hotspot/share/prims/resolvedMethodTable.cpp
     No comments.


Thumbs up! I only have minor nits above. It is your call whether
you want to fix them in a follow bug.

Dan


> bug link https://bugs.openjdk.java.net/browse/JDK-8078725
>
> Thanks,
> Coleen
>


[prev in list] [next in list] [prev in thread] [next in thread]