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.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
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
