RFR: 8352075: Perf regression accessing fields [v3]

[Frederic Parain]

On Fri, 25 Apr 2025 13:05:51 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> Radim Vansa has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - Fix VerifyRawIndexesTest
>>  - Fix reordering in layout and annotations
>>  - Use qsort_r for different platforms
>
> src/hotspot/share/oops/fieldInfo.hpp line 290:
> 
>> 288:   static int compare_symbols(const Symbol *s1, const Symbol *s2);
>> 289: 
>> 290:   static Array<u1>* create_FieldInfoStream(ConstantPool* constants, GrowableArray<FieldInfo>* fields, int java_fields, int injected_fields,
> 
> In the latest form the ConstantPool parameter is used only for assertion, though I think that it is rather important assertion.

The ConstantPool parameter can be limited to debug builds (the ones with asserts) with the following patch:


diff --git a/src/hotspot/share/classfile/classFileParser.cpp b/src/hotspot/share/classfile/classFileParser.cpp
index 48646c0fb83..19471bbf7ee 100644
--- a/src/hotspot/share/classfile/classFileParser.cpp
+++ b/src/hotspot/share/classfile/classFileParser.cpp
@@ -5813,7 +5813,7 @@ void ClassFileParser::post_process_parsed_stream(const ClassFileStream* const st
 
   int injected_fields_count = _temp_field_info->length() - _java_fields_count;
   _fieldinfo_stream =
-    FieldInfoStream::create_FieldInfoStream(_cp, _temp_field_info, _java_fields_count,
+    FieldInfoStream::create_FieldInfoStream(DEBUG_ONLY(_cp COMMA) _temp_field_info, _java_fields_count,
                                             injected_fields_count, loader_data(), CHECK);
   _fields_status =
     MetadataFactory::new_array<FieldStatus>(_loader_data, _temp_field_info->length(),
diff --git a/src/hotspot/share/classfile/javaClasses.cpp b/src/hotspot/share/classfile/javaClasses.cpp
index f3fdf28b96b..80ee179576c 100644
--- a/src/hotspot/share/classfile/javaClasses.cpp
+++ b/src/hotspot/share/classfile/javaClasses.cpp
@@ -963,7 +963,7 @@ void java_lang_Class::fixup_mirror(Klass* k, TRAPS) {
       }
       Array<u1>* old_stream = ik->fieldinfo_stream();
       assert(fields->length() == (java_fields + injected_fields), "Must be");
-      Array<u1>* new_fis = FieldInfoStream::create_FieldInfoStream(ik->constants(), fields, java_fields, injected_fields, k->class_loader_data(), CHECK);
+      Array<u1>* new_fis = FieldInfoStream::create_FieldInfoStream(DEBUG_ONLY(ik->constants() COMMA) fields, java_fields, injected_fields, k->class_loader_data(), CHECK);
       ik->set_fieldinfo_stream(new_fis);
       MetadataFactory::free_array<u1>(k->class_loader_data(), old_stream);
     }
diff --git a/src/hotspot/share/oops/fieldInfo.cpp b/src/hotspot/share/oops/fieldInfo.cpp
index dd1fa11042d..eb90e6bdae8 100644
--- a/src/hotspot/share/oops/fieldInfo.cpp
+++ b/src/hotspot/share/oops/fieldInfo.cpp
@@ -66,7 +66,7 @@ int FieldInfoStream::compare_symbols(const Symbol *s1, const Symbol *s2) {
   }
 }
 
-Array<u1>* FieldInfoStream::create_FieldInfoStream(ConstantPool* constants, GrowableArray<FieldInfo>* fields, int java_fields, int injected_fields,
+Array<u1>* FieldInfoStream::create_FieldInfoStream(DEBUG_ONLY(ConstantPool* constants COMMA) GrowableArray<FieldInfo>* fields, int java_fields, int injected_fields,
                                                           ClassLoaderData* loader_data, TRAPS) {
   // The stream format described in fieldInfo.hpp is:
   //   FieldInfoStream := j=num_java_fields k=num_injected_fields JumpTable_offset(0/4 bytes) Field[j+k] JumpTable[(j - 1)/16 > 0] End
diff --git a/src/hotspot/share/oops/fieldInfo.hpp b/src/hotspot/share/oops/fieldInfo.hpp
index 1974e2e4117..b98d9230533 100644
--- a/src/hotspot/share/oops/fieldInfo.hpp
+++ b/src/hotspot/share/oops/fieldInfo.hpp
@@ -287,7 +287,7 @@ class FieldInfoStream : AllStatic {
 
   static int compare_symbols(const Symbol *s1, const Symbol *s2);
 
-  static Array<u1>* create_FieldInfoStream(ConstantPool* constants, GrowableArray<FieldInfo>* fields, int java_fields, int injected_fields,
+  static Array<u1>* create_FieldInfoStream(DEBUG_ONLY(ConstantPool* constants COMMA) GrowableArray<FieldInfo>* fields, int java_fields, int injected_fields,
                                                           ClassLoaderData* loader_data, TRAPS);
   static GrowableArray<FieldInfo>* create_FieldInfoArray(const Array<u1>* fis, int* java_fields_count, int* injected_fields_count);
   static void print_from_fieldinfo_stream(Array<u1>* fis, outputStream* os, ConstantPool* cp);
diff --git a/src/hotspot/share/prims/jvmtiRedefineClasses.cpp b/src/hotspot/share/prims/jvmtiRedefineClasses.cpp
index 4d50d6cb637..46ffe767448 100644
--- a/src/hotspot/share/prims/jvmtiRedefineClasses.cpp
+++ b/src/hotspot/share/prims/jvmtiRedefineClasses.cpp
@@ -3555,7 +3555,7 @@ void VM_RedefineClasses::set_new_constant_pool(
   if (update_required) {
     Array<u1>* old_stream = scratch_class->fieldinfo_stream();
     assert(fields->length() == (java_fields + injected_fields), "Must be");
-    Array<u1>* new_fis = FieldInfoStream::create_FieldInfoStream(cp, fields, java_fields, injected_fields, scratch_class->class_loader_data(), CHECK);
+    Array<u1>* new_fis = FieldInfoStream::create_FieldInfoStream(DEBUG_ONLY(cp COMMA) fields, java_fields, injected_fields, scratch_class->class_loader_data(), CHECK);
     scratch_class->set_fieldinfo_stream(new_fis);
     MetadataFactory::free_array<u1>(scratch_class->class_loader_data(), old_stream);
   }

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24847#discussion_r2069379965