On Tue, 13 May 2025 11:39:32 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> Radim Vansa has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Move constant to static final var
>
> Also can you include this:
>
> diff --git a/src/hotspot/share/oops/fieldInfo.cpp
> b/src/hotspot/share/oops/fieldInfo.cpp
> index 300b45277ad..7668faf6187 100644
> --- a/src/hotspot/share/oops/fieldInfo.cpp
> +++ b/src/hotspot/share/oops/fieldInfo.cpp
> @@ -37,8 +37,8 @@ void FieldInfo::print(outputStream* os, ConstantPool* cp) {
> field_flags().as_uint(),
> initializer_index(),
> generic_signature_index(),
> - _field_flags.is_injected() ?
> lookup_symbol(generic_signature_index())->as_utf8() :
> cp->symbol_at(generic_signature_index())->as_utf8(),
> - contended_group());
> + field_flags().is_generic() ?
> cp->symbol_at(generic_signature_index())->as_utf8() : "",
> + is_contended() ? contended_group() : 0);
> }
>
>
> which I fixed for printing.
>
> Also, sorting the fields is inlined code in parse_fields. Can you make it
> it's own method? And it seems to sort the fields before the injected fields
> are added, so how do you find the injected fields? I think injected fields
> aren't added to klasses with >16 fields maybe?
>
> I was going to write a test with JVMTI GetFields and > 16 fields because I'm
> not sure how/if you're fixing the order that the fields are returned by that
> API.
@coleenp @SirYwell Based on the feedback and considerations from your reviews,
I've pushed a version that does not reorder fields. The `CCC` test shows the
same performance as the original PR, in case of huge class the improvement is
even better (0.23 s vs 0.5 s). The price for O(log(N)) field lookup is 3 bytes
per field for medium classes (17 - 256 non-injected fields), 4-5 bytes for
bigger classes.
Due to not reordering the fields at all the chance for regression is
significantly lower. I have not executed the whole JCK but tried the mentioned
test and it does pass.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24847#issuecomment-2880137455
