RFR: 8352075: Perf regression accessing fields [v3]

[Coleen Phillimore]

On Wed, 30 Apr 2025 20:15:36 GMT, Frederic Parain <fparain at openjdk.org> wrote:

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

I don't think adding DEBUG_ONLY optimizes anything and kind of looks messy, I don't like this change unless the product compiler complains about an unused parameter.

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

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