RFR (M) 8078725: method adjustments can be done just once for all classes involved into redefinition
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Feb 25 19:00:45 UTC 2019
On 2/25/19 12:40 PM, Daniel D. Daugherty wrote:
> 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 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
>
> 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.
Yes, that is always the case. For this adjustment, we never adjust the
method entry from a different class for is_old() methods.
>
> 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.
Yes. All entries are now interesting.
>
> 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");
>
Yes, there were a lot of instances of this same pattern.
> src/hotspot/share/prims/jvmtiRedefineClasses.hpp
> L356: Klass* _the_class;
> '_the_class' is no longer static. Interesting...
> Any particular reason?
>
It doesn't need to be and it won't have the right value in Closures that
we use ClassLoaderDataGraph::classes_do() so we must not use it.
> L518: // to fix up these pointers and clean MethodData out
> nit - missing a '.' at the end. Serguei may have flagged this.
>
Yes, I got 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 '.'
Two spaces after a period is proper English but I can remove it.
>
> L3426: // Adjust cpools and vtables closure
> L3427: void
> VM_RedefineClasses::AdjustAndCleanMetadata::do_klass(Klass* k) {
> Comment needs adjusting to the new class name.
I removed 3426 comment, because it's redundant with the comment just
above it.
>
> 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.
Yes, this optimization was worth keeping.
>
> 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.
Yes, we don't look at the method names anymore or really the set of
classes. This walk is done after all the methods that have been
redefined are marked is_old(), and that is the set of methods we have to
adjust.
>
> 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.
That was another optimization worth keeping.
>
> 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.
Yes, we couldn't keep these optimizations, unless written as
_is_any_class_redefined_an_interface and
is_any_class_redefined_a_subtype_of(k). If we wanted to optimize
further, we could add these functions or this 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.
Yes.
>
> 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.
>
I would fix them in a following bug but it was a small set. I'll fix
them when I'm in the file next, which might be soon.
Thank you for the thorough code review and walking through the change.
Sorry I didn't wait but I didn't know when you'd find this in your
vacation email backlog.
thanks!
Coleen
> Dan
>
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8078725
>>
>> Thanks,
>> Coleen
>>
>
More information about the hotspot-runtime-dev
mailing list