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

[Aleksey Shipilev]

On Thu, 10 Apr 2025 07:11:46 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:
> 
>   Cleanup after initial review

Looks reasonable to me, just the stylistic nits. @fparain should take a look as well.

src/hotspot/share/oops/fieldStreams.hpp line 134:

> 132:   fieldDescriptor& field_descriptor() const {
> 133:     fieldDescriptor& field = const_cast<fieldDescriptor&>(_fd_buf);
> 134:     field.reinitialize(field_holder(), _fi_buf);

`_fi_buf` - > `to_FieldInfo()`? Reads better, and I expect it to fully inline without any performance loss.

src/hotspot/share/runtime/fieldDescriptor.hpp line 105:

> 103: 
> 104:   // Initialization
> 105:   void reinitialize(InstanceKlass* ik, const FieldInfo &fieldinfo);

I think the style is `const FieldInfo& fieldinfo`. Also in definition.

src/hotspot/share/utilities/tuple.hpp line 43:

> 41: 
> 42: public:
> 43:   constexpr Tuple() noexcept {}

Do you still need this?

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

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24290#pullrequestreview-2755649254
PR Review Comment: https://git.openjdk.org/jdk/pull/24290#discussion_r2036713643
PR Review Comment: https://git.openjdk.org/jdk/pull/24290#discussion_r2036718573
PR Review Comment: https://git.openjdk.org/jdk/pull/24290#discussion_r2036703719