RFR: 8353175: Eliminate double iteration of stream in FieldDescriptor reinitialization [v2]

[Johan Sjölen]

On Fri, 28 Mar 2025 12:02:58 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> On the reproducer https://bugs.openjdk.org/secure/attachment/113985/CCC.java my local testing shows these numbers:
>> 
>> ### JDK-17
>> 
>> $ hyperfine -w 5 -r 10 '/path/to/jdk-17/bin/java -cp /tmp/ CCC'
>> Benchmark 1: /path/to/jdk-17/bin/java -cp /tmp/ CCC
>>   Time (mean ± σ):      32.5 ms ±   0.9 ms    [User: 27.5 ms, System: 10.6 ms]
>>   Range (min … max):    31.1 ms …  33.7 ms    10 runs
>> 
>> ### JDK-25 before the change applied
>> 
>> $ hyperfine -w 5 -r 10 '/path/to/jdk-25/build/linux-x86_64-server-release/images/jdk/bin/java -cp /tmp/ CCC'
>> Benchmark 1: /path/to/jdk-25/build/linux-x86_64-server-release/images/jdk/bin/java -cp /tmp/ CCC
>>   Time (mean ± σ):     101.6 ms ±   1.5 ms    [User: 96.8 ms, System: 14.6 ms]
>>   Range (min … max):    99.0 ms … 104.5 ms    10 runs
>> 
>> ### JDK-25 with this patch
>> 
>> $ hyperfine -w 5 -r 10 '/path/to/jdk-25/build/linux-x86_64-server-release/images/jdk/bin/java -cp /tmp/ CCC'
>> Benchmark 1: /path/to/jdk-25/build/linux-x86_64-server-release/images/jdk/bin/java -cp /tmp/ CCC
>>   Time (mean ± σ):      75.8 ms ±   1.2 ms    [User: 69.8 ms, System: 16.0 ms]
>>   Range (min … max):    73.8 ms …  78.2 ms    10 runs
>
> Radim Vansa has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix compilation error in assertion

Hi,

The idea behind the change is good, but I think that we can clean up the code. See the comments for how that can be done.

src/hotspot/share/oops/instanceKlass.cpp line 1940:

> 1938:   // In DebugInfo nonstatic fields are sorted by offset.
> 1939:   GrowableArray<Tuple<int, int, FieldInfo> > fields_sorted;
> 1940:   int i = 0;

Would you mind also cleaning up this usage of `i`? Seems like it can be removed and `fields_sorted.length()` can be used instead.

src/hotspot/share/oops/instanceKlass.cpp line 1944:

> 1942:     if (!fs.access_flags().is_static()) {
> 1943:       fd = fs.field_descriptor();
> 1944:       Tuple<int, int, FieldInfo> f(fs.offset(), fs.index(), fs.to_FieldInfo());

`FieldInfo` contains the `offset`, so that's not necessary. Besides, the `index` is now only used for an `assert` in the `reinitialize` code. Why not get rid of the index argument and the assert?

In other words: Get rid of the `Tuple` changes and have `fields_sorted` be a `GrowableArray` of `FiedlInfo` only.

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

PR Review: https://git.openjdk.org/jdk/pull/24290#pullrequestreview-2752454462
PR Review Comment: https://git.openjdk.org/jdk/pull/24290#discussion_r2034713775
PR Review Comment: https://git.openjdk.org/jdk/pull/24290#discussion_r2034738604