8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields

Reviewed-by: yzheng, never, thartmann

Author: Doug Simon

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;
 }

src/hotspot/share/ci/ciInstanceKlass.hpp

@@ -67,7 +67,7 @@ class ciInstanceKlass : public ciKlass {
   ciInstance*            _java_mirror;
 
   ciConstantPoolCache*   _field_cache;  // cached map index->field
-  GrowableArray<ciField*>* _nonstatic_fields;
+  GrowableArray<ciField*>* _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:

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 @@ class ReassignedField {
   }
 };
 
-static int compare(ReassignedField* left, ReassignedField* right) {
-  return left->_offset - right->_offset;
-}
-
-// 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<ReassignedField>* fields = new GrowableArray<ReassignedField>();
-  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);
-      }
+// Gets the fields of `klass` that are eliminated by escape analysis and need to be reassigned
+static GrowableArray<ReassignedField>* get_reassigned_fields(InstanceKlass* klass, GrowableArray<ReassignedField>* 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);
     }
-    ik = ik->superklass();
   }
-  fields->sort(compare);
+  return fields;
+}
+
+// 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<ReassignedField>* fields = get_reassigned_fields(klass, new GrowableArray<ReassignedField>(), 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<ScopeValue*>* objects, bool realloc_failures, bool skip_internal) {
+void Deoptimization::reassign_fields(frame* fr, RegisterMap* reg_map, GrowableArray<ScopeValue*>* 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());

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 @@ public boolean isStatic() {
         }
     }
 
-    static class SortByOffset implements Comparator<ResolvedJavaField> {
-        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 @@ public ResolvedJavaField[] getInstanceFields(boolean includeSuperclasses) {
                         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 @@ private HotSpotResolvedJavaField[] getFields(boolean retrieveStaticFields, HotSp
                 result[resultIndex++] = resolvedJavaField;
             }
         }
-        Arrays.sort(result, fieldSortingMethod);
         return result;
     }
 

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaType.java

@@ -284,24 +284,21 @@ default ResolvedJavaMethod resolveConcreteMethod(ResolvedJavaMethod method, Reso
     AssumptionResult<ResolvedJavaMethod> 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();
 

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.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 @@ private static Class<? extends Annotation> findPolymorphicSignatureClass() {
     public void findInstanceFieldWithOffsetTest() {
         for (Class<?> c : classes) {
             ResolvedJavaType type = metaAccess.lookupJavaType(c);
-            Set<Field> reflectionFields = getInstanceFields(c, true);
+            Set<Field> reflectionFields = Set.copyOf(getInstanceFields(c, true));
             for (Field f : reflectionFields) {
                 ResolvedJavaField rf = lookupField(type.getInstanceFields(true), f);
                 assertNotNull(rf);
@@ -872,18 +874,20 @@ public void findUniqueConcreteMethodTest() throws NoSuchMethodException {
         assertEquals(thisMethod, ucm);
     }
 
-    public static Set<Field> getInstanceFields(Class<?> c, boolean includeSuperclasses) {
+    public static List<Field> getInstanceFields(Class<?> c, boolean includeSuperclasses) {
         if (c.isArray() || c.isPrimitive() || c.isInterface()) {
-            return Collections.emptySet();
+            return List.of();
         }
-        Set<Field> result = new HashSet<>();
+        List<Field> 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<Field> allFields = getInstanceFields(c.getSuperclass(), true);
+            allFields.addAll(result);
+            result = allFields;
         }
         return result;
     }
@@ -912,7 +916,7 @@ public ResolvedJavaField lookupField(ResolvedJavaField[] fields, Field key) {
         return null;
     }
 
-    public Field lookupField(Set<Field> fields, ResolvedJavaField key) {
+    public Field lookupField(Collection<Field> fields, ResolvedJavaField key) {
         for (Field f : fields) {
             if (fieldsEqual(f, key)) {
                 return f;
@@ -922,9 +926,6 @@ public Field lookupField(Set<Field> fields, ResolvedJavaField key) {
     }
 
     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 void getInstanceFieldsTest() {
         for (Class<?> c : classes) {
             ResolvedJavaType type = metaAccess.lookupJavaType(c);
             for (boolean includeSuperclasses : new boolean[]{true, false}) {
-                Set<Field> expected = getInstanceFields(c, includeSuperclasses);
-                ResolvedJavaField[] actual = type.getInstanceFields(includeSuperclasses);
-                for (Field f : expected) {
-                    assertNotNull(lookupField(actual, f));
+                List<Field> 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);
             }
         }
     }