On Thu, 5 Jun 2025 20:52:10 GMT, Radim Vansa <rva...@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
