RFR: 8352075: Perf regression accessing fields [v21]

[Coleen Phillimore]

On Thu, 5 Jun 2025 20:52:10 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> src/hotspot/share/oops/fieldInfo.cpp line 164:
>> 
>>> 162:   r.read_field_counts(&java_fields, &injected_fields);
>>> 163:   assert(java_fields >= 0, "must be");
>>> 164:   if (java_fields == 0 || fis->length() == 0 || static_cast<uint>(java_fields) < BinarySearchThreshold) {
>> 
>> I don't know why you only sort Java fields and ignore the injected fields.  JavaClasses::compute_offsets calls find_local_field, so might not find an injected field, I assume in the java.lang.Class (mirror).  Should this sorted cache exclude classes with injected fields? ie if injected_fields > 0?
>> If you exclude classes with injected fields, you could remove the javaClasses code (and maybe not have to re-sort any fields during dynamic dumping (?))
>
> I don't build a search table for injected fields because I am trying to fix performance of `InstanceKlass::find_local_field` and this uses `JavaFieldStream` - that is/was ignoring injected fields in the iteration as well.
> Classes with injected fields are not excluded, we just don't build the table for them. There's not lookup by name+signature, just `InstanceKlass::field(int index)` which uses iteration through `AllFieldStream`.

Okay the code I looked at in JavaClasses calls AllFieldStream for injected fields.

>> src/hotspot/share/oops/fieldInfo.cpp line 285:
>> 
>>> 283:     FieldInfo fi;
>>> 284:     reader.read_field_info(fi);
>>> 285:     if (fi.field_flags().is_injected()) {
>> 
>> I thought that above, you only process java fields and not the injected fields?
>
> `FieldInfoReader` is limited by the full stream, and after iterating through java fields it would start returning injected fields. For java fields we call the lookup below; we know that injected fields don't have a record in the table, and we know that there won't be any more java fields after we encounter the first injected field; that's why we `break` the cycle here.

Okay, this does have a comment about this assumption.

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

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