On Thu, 5 Jun 2025 18:54:07 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> Radim Vansa has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Moved jtreg test
>> - Improved documentation
>> - Fix coding style (asterisk placement)
>
> 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`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24847#discussion_r2130327017
