From 0cb110ebb7f8d184dd855f64c5dd7924c8202b3d Mon Sep 17 00:00:00 2001 From: Doug Simon Date: Fri, 21 Mar 2025 13:00:25 +0000 Subject: [PATCH] 8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields Reviewed-by: yzheng, never, thartmann --- src/hotspot/share/ci/ciInstanceKlass.cpp | 11 ----- src/hotspot/share/ci/ciInstanceKlass.hpp | 2 +- src/hotspot/share/runtime/deoptimization.cpp | 46 +++++++++---------- .../HotSpotResolvedObjectTypeImpl.java | 9 ---- .../jdk/vm/ci/meta/ResolvedJavaType.java | 17 +++---- .../ci/runtime/test/TestResolvedJavaType.java | 43 ++++++++++------- 6 files changed, 57 insertions(+), 71 deletions(-) diff --git a/src/hotspot/share/ci/ciInstanceKlass.cpp b/src/hotspot/share/ci/ciInstanceKlass.cpp index 5799f30ad67..e8d71b32b68 100644 --- a/src/hotspot/share/ci/ciInstanceKlass.cpp +++ b/src/hotspot/share/ci/ciInstanceKlass.cpp @@ -400,9 +400,6 @@ ciField* ciInstanceKlass::get_field_by_offset(int field_offset, bool is_static) int field_off = field->offset_in_bytes(); if (field_off == field_offset) return field; - if (field_off > field_offset) - break; - // could do binary search or check bins, but probably not worth it } return nullptr; } @@ -431,11 +428,6 @@ ciField* ciInstanceKlass::get_field_by_name(ciSymbol* name, ciSymbol* signature, } -static int sort_field_by_offset(ciField** a, ciField** b) { - return (*a)->offset_in_bytes() - (*b)->offset_in_bytes(); - // (no worries about 32-bit overflow...) -} - // ------------------------------------------------------------------ // ciInstanceKlass::compute_nonstatic_fields int ciInstanceKlass::compute_nonstatic_fields() { @@ -476,9 +468,6 @@ int ciInstanceKlass::compute_nonstatic_fields() { int flen = fields->length(); - // Now sort them by offset, ascending. - // (In principle, they could mix with superclass fields.) - fields->sort(sort_field_by_offset); _nonstatic_fields = fields; return flen; } diff --git a/src/hotspot/share/ci/ciInstanceKlass.hpp b/src/hotspot/share/ci/ciInstanceKlass.hpp index 9c1c416780d..69b73152d37 100644 --- a/src/hotspot/share/ci/ciInstanceKlass.hpp +++ b/src/hotspot/share/ci/ciInstanceKlass.hpp @@ -67,7 +67,7 @@ private: ciInstance* _java_mirror; ciConstantPoolCache* _field_cache; // cached map index->field - GrowableArray* _nonstatic_fields; + GrowableArray* _nonstatic_fields; // ordered by JavaFieldStream int _has_injected_fields; // any non static injected fields? lazily initialized. // The possible values of the _implementor fall into following three cases: diff --git a/src/hotspot/share/runtime/deoptimization.cpp b/src/hotspot/share/runtime/deoptimization.cpp index 748cd0d7939..f7e0844639b 100644 --- a/src/hotspot/share/runtime/deoptimization.cpp +++ b/src/hotspot/share/runtime/deoptimization.cpp @@ -377,8 +377,9 @@ static bool rematerialize_objects(JavaThread* thread, int exec_mode, nmethod* co realloc_failures = Deoptimization::realloc_objects(thread, &deoptee, &map, objects, THREAD); JRT_END } - bool skip_internal = (compiled_method != nullptr) && !compiled_method->is_compiled_by_jvmci(); - Deoptimization::reassign_fields(&deoptee, &map, objects, realloc_failures, skip_internal); + guarantee(compiled_method != nullptr, "deopt must be associated with an nmethod"); + bool is_jvmci = compiled_method->is_compiled_by_jvmci(); + Deoptimization::reassign_fields(&deoptee, &map, objects, realloc_failures, is_jvmci); if (TraceDeoptimization) { print_objects(deoptee_thread, objects, realloc_failures); } @@ -1458,27 +1459,26 @@ public: } }; -static int compare(ReassignedField* left, ReassignedField* right) { - return left->_offset - right->_offset; +// Gets the fields of `klass` that are eliminated by escape analysis and need to be reassigned +static GrowableArray* get_reassigned_fields(InstanceKlass* klass, GrowableArray* fields, bool is_jvmci) { + InstanceKlass* super = klass->superklass(); + if (super != nullptr) { + get_reassigned_fields(super, fields, is_jvmci); + } + for (AllFieldStream fs(klass); !fs.done(); fs.next()) { + if (!fs.access_flags().is_static() && (is_jvmci || !fs.field_flags().is_injected())) { + ReassignedField field; + field._offset = fs.offset(); + field._type = Signature::basic_type(fs.signature()); + fields->append(field); + } + } + return fields; } -// Restore fields of an eliminated instance object using the same field order -// returned by HotSpotResolvedObjectTypeImpl.getInstanceFields(true) -static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap* reg_map, ObjectValue* sv, int svIndex, oop obj, bool skip_internal) { - GrowableArray* fields = new GrowableArray(); - InstanceKlass* ik = klass; - while (ik != nullptr) { - for (AllFieldStream fs(ik); !fs.done(); fs.next()) { - if (!fs.access_flags().is_static() && (!skip_internal || !fs.field_flags().is_injected())) { - ReassignedField field; - field._offset = fs.offset(); - field._type = Signature::basic_type(fs.signature()); - fields->append(field); - } - } - ik = ik->superklass(); - } - fields->sort(compare); +// Restore fields of an eliminated instance object employing the same field order used by the compiler. +static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap* reg_map, ObjectValue* sv, int svIndex, oop obj, bool is_jvmci) { + GrowableArray* fields = get_reassigned_fields(klass, new GrowableArray(), is_jvmci); for (int i = 0; i < fields->length(); i++) { ScopeValue* scope_field = sv->field_at(svIndex); StackValue* value = StackValue::create_stack_value(fr, reg_map, scope_field); @@ -1560,7 +1560,7 @@ static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap } // restore fields of all eliminated objects and arrays -void Deoptimization::reassign_fields(frame* fr, RegisterMap* reg_map, GrowableArray* objects, bool realloc_failures, bool skip_internal) { +void Deoptimization::reassign_fields(frame* fr, RegisterMap* reg_map, GrowableArray* objects, bool realloc_failures, bool is_jvmci) { for (int i = 0; i < objects->length(); i++) { assert(objects->at(i)->is_object(), "invalid debug information"); ObjectValue* sv = (ObjectValue*) objects->at(i); @@ -1604,7 +1604,7 @@ void Deoptimization::reassign_fields(frame* fr, RegisterMap* reg_map, GrowableAr } if (k->is_instance_klass()) { InstanceKlass* ik = InstanceKlass::cast(k); - reassign_fields_by_klass(ik, fr, reg_map, sv, 0, obj(), skip_internal); + reassign_fields_by_klass(ik, fr, reg_map, sv, 0, obj(), is_jvmci); } else if (k->is_typeArray_klass()) { TypeArrayKlass* ak = TypeArrayKlass::cast(k); reassign_type_array_elements(fr, reg_map, sv, (typeArrayOop) obj(), ak->element_type()); diff --git a/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java b/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java index 91c9e73b532..8d3b9f48b4c 100644 --- a/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java +++ b/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java @@ -67,7 +67,6 @@ final class HotSpotResolvedObjectTypeImpl extends HotSpotResolvedJavaType implem private static final HotSpotResolvedJavaField[] NO_FIELDS = new HotSpotResolvedJavaField[0]; private static final int METHOD_CACHE_ARRAY_CAPACITY = 8; - private static final SortByOffset fieldSortingMethod = new SortByOffset(); /** * The {@code Klass*} of this type. @@ -782,12 +781,6 @@ final class HotSpotResolvedObjectTypeImpl extends HotSpotResolvedJavaType implem } } - static class SortByOffset implements Comparator { - public int compare(ResolvedJavaField a, ResolvedJavaField b) { - return a.getOffset() - b.getOffset(); - } - } - @Override public ResolvedJavaField[] getInstanceFields(boolean includeSuperclasses) { if (instanceFields == null) { @@ -817,7 +810,6 @@ final class HotSpotResolvedObjectTypeImpl extends HotSpotResolvedJavaType implem result[i++] = f; } } - Arrays.sort(result, fieldSortingMethod); return result; } else { // The super classes of this class do not have any instance fields. @@ -876,7 +868,6 @@ final class HotSpotResolvedObjectTypeImpl extends HotSpotResolvedJavaType implem result[resultIndex++] = resolvedJavaField; } } - Arrays.sort(result, fieldSortingMethod); return result; } diff --git a/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaType.java b/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaType.java index 272f821b7ca..b5d29733a69 100644 --- a/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaType.java +++ b/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaType.java @@ -284,24 +284,21 @@ public interface ResolvedJavaType extends JavaType, ModifiersProvider, Annotated AssumptionResult findUniqueConcreteMethod(ResolvedJavaMethod method); /** - * Returns the instance fields of this class, including + * Returns the non-static fields of this class, including * {@linkplain ResolvedJavaField#isInternal() internal} fields. A zero-length array is returned - * for array and primitive types. The order of fields returned by this method is stable. That - * is, for a single JVM execution the same order is returned each time this method is called. It - * is also the "natural" order, which means that the JVM would expect the fields in this order - * if no specific order is given. + * for array and primitive types. The order of fields declared by a single class returned by + * this method is the same as {@link Class#getDeclaredFields}. * - * @param includeSuperclasses if true, then instance fields for the complete hierarchy of this - * type are included in the result - * @return an array of instance fields + * @param includeSuperclasses if true, then non-static fields for the complete hierarchy of this + * type are included in the result with superclass fields coming before subclass fields + * @return an array of non-static fields */ ResolvedJavaField[] getInstanceFields(boolean includeSuperclasses); /** * Returns the static fields of this class, including {@linkplain ResolvedJavaField#isInternal() * internal} fields. A zero-length array is returned for array and primitive types. The order of - * fields returned by this method is stable. That is, for a single JVM execution the same order - * is returned each time this method is called. + * fields returned by this method is the same as {@link Class#getDeclaredFields}. */ ResolvedJavaField[] getStaticFields(); diff --git a/test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java b/test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java index 52b52cf647f..456c4a5fd88 100644 --- a/test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java +++ b/test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java @@ -71,7 +71,9 @@ import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.function.BiConsumer; import java.util.function.Supplier; @@ -144,7 +146,7 @@ public class TestResolvedJavaType extends TypeUniverse { public void findInstanceFieldWithOffsetTest() { for (Class c : classes) { ResolvedJavaType type = metaAccess.lookupJavaType(c); - Set reflectionFields = getInstanceFields(c, true); + Set reflectionFields = Set.copyOf(getInstanceFields(c, true)); for (Field f : reflectionFields) { ResolvedJavaField rf = lookupField(type.getInstanceFields(true), f); assertNotNull(rf); @@ -872,18 +874,20 @@ public class TestResolvedJavaType extends TypeUniverse { assertEquals(thisMethod, ucm); } - public static Set getInstanceFields(Class c, boolean includeSuperclasses) { + public static List getInstanceFields(Class c, boolean includeSuperclasses) { if (c.isArray() || c.isPrimitive() || c.isInterface()) { - return Collections.emptySet(); + return List.of(); } - Set result = new HashSet<>(); + List result = new ArrayList<>(); for (Field f : c.getDeclaredFields()) { if (!Modifier.isStatic(f.getModifiers())) { result.add(f); } } if (includeSuperclasses && c != Object.class) { - result.addAll(getInstanceFields(c.getSuperclass(), true)); + List allFields = getInstanceFields(c.getSuperclass(), true); + allFields.addAll(result); + result = allFields; } return result; } @@ -912,7 +916,7 @@ public class TestResolvedJavaType extends TypeUniverse { return null; } - public Field lookupField(Set fields, ResolvedJavaField key) { + public Field lookupField(Collection fields, ResolvedJavaField key) { for (Field f : fields) { if (fieldsEqual(f, key)) { return f; @@ -922,9 +926,6 @@ public class TestResolvedJavaType extends TypeUniverse { } private static boolean isHiddenFromReflection(ResolvedJavaField f) { - if (f.getDeclaringClass().equals(metaAccess.lookupJavaType(Throwable.class)) && f.getName().equals("backtrace")) { - return true; - } if (f.getDeclaringClass().equals(metaAccess.lookupJavaType(ConstantPool.class)) && f.getName().equals("constantPoolOop")) { return true; } @@ -955,20 +956,28 @@ public class TestResolvedJavaType extends TypeUniverse { for (Class c : classes) { ResolvedJavaType type = metaAccess.lookupJavaType(c); for (boolean includeSuperclasses : new boolean[]{true, false}) { - Set expected = getInstanceFields(c, includeSuperclasses); - ResolvedJavaField[] actual = type.getInstanceFields(includeSuperclasses); - for (Field f : expected) { - assertNotNull(lookupField(actual, f)); + List reflectFields = getInstanceFields(c, includeSuperclasses); + ResolvedJavaField[] fields = type.getInstanceFields(includeSuperclasses); + int reflectFieldIndex = 0; + for (int i = 0; i < fields.length; i++) { + ResolvedJavaField field = fields[i]; + if (field.isInternal() || isHiddenFromReflection(field)) { + continue; + } + Field reflectField = reflectFields.get(reflectFieldIndex++); + ResolvedJavaField field2 = lookupField(fields, reflectField); + + assertEquals("ResolvedJavaType.getInstanceFields order differs from Class.getDeclaredFields", field, field2); } - for (ResolvedJavaField rf : actual) { + for (ResolvedJavaField rf : fields) { if (!isHiddenFromReflection(rf)) { - assertEquals(rf.toString(), lookupField(expected, rf) != null, !rf.isInternal()); + assertEquals(rf.toString(), lookupField(reflectFields, rf) != null, !rf.isInternal()); } } // Test stability of getInstanceFields - ResolvedJavaField[] actual2 = type.getInstanceFields(includeSuperclasses); - assertArrayEquals(actual, actual2); + ResolvedJavaField[] fields2 = type.getInstanceFields(includeSuperclasses); + assertArrayEquals(fields, fields2); } } }