RFR: 8352075: Perf regression accessing fields [v23]

[Coleen Phillimore]

On Fri, 6 Jun 2025 07:14:21 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> This optimization is a followup to https://github.com/openjdk/jdk/pull/24290 trying to reduce the performance regression in some scenarios introduced in https://bugs.openjdk.org/browse/JDK-8292818 . Based both on performance and memory consumption it is a (better) alternative to https://github.com/openjdk/jdk/pull/24713 .
>> 
>> This PR optimizes local field lookup in classes with more than 16 fields; rather than sequentially iterating through all fields during lookup we sort the fields based on the field name. The stream includes extra table after the field information: for field at position 16, 32 ... we record the (variable-length-encoded) offset of the field info in this stream. On field lookup, rather than iterating through all fields, we iterate through this table, resolve names for given fields and continue field-by-field iteration only after the last record (hence at most 16 fields).
>> 
>> In classes with <= 16 fields this PR reduces the memory consumption by 1 byte that was left with value 0 at the end of stream. In classes with > 16 fields we add extra 4 bytes with offset of the table, and the table contains one varint for each 16 fields. The terminal byte is not used either.
>> 
>> My measurements on the attached reproducer
>> 
>> hyperfine -w 50 -r 100 '/path/to/jdk-17/bin/java -cp /tmp CCC'
>> Benchmark 1: /path/to/jdk-17/bin/java -cp /tmp CCC
>>   Time (mean ± σ):      51.3 ms ±   2.8 ms    [User: 44.7 ms, System: 13.7 ms]
>>   Range (min … max):    45.1 ms …  53.9 ms    100 runs
>> 
>> hyperfine -w 50 -r 100 '/path/to/jdk25-master/bin/java -cp /tmp CCC'
>> Benchmark 1: /path/to/jdk25-master/bin/java -cp /tmp CCC
>>   Time (mean ± σ):      78.2 ms ±   1.0 ms    [User: 74.6 ms, System: 17.3 ms]
>>   Range (min … max):    73.8 ms …  79.7 ms    100 runs
>> 
>> (the jdk25-master above already contains JDK-8353175)
>> 
>> hyperfine -w 50 -r 100 '/path/to/jdk25-this-pr/bin/java -cp /tmp CCC'
>> Benchmark 1: /path/to/jdk25-this-pr/jdk/bin/java -cp /tmp CCC
>>   Time (mean ± σ):      38.5 ms ±   0.5 ms    [User: 34.4 ms, System: 17.3 ms]
>>   Range (min … max):    37.7 ms …  42.1 ms    100 runs
>> 
>> While https://github.com/openjdk/jdk/pull/24713 returned the performance to previous levels, this PR improves it by 25% compared to JDK 17 (which does not contain the regression)! This time, the undisclosed production-grade reproducer shows even higher improvement:
>> 
>> JDK 17: 1.6 s
>> JDK 21 (no patches): 22 s
>> JDK25-master: 12.3 s
>> JDK25-this-pr: 0.5 s
>
> Radim Vansa has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Add more comments
>  - Disable search table with dynamic CDS

I have a couple more comments for today.

src/hotspot/share/utilities/packedTable.cpp line 49:

> 47:     assert((key & ~_key_mask) == 0, "key out of bounds");
> 48:     assert((value & ~_value_mask) == 0, "value out of bounds: %x vs. %x (%x)", value, _value_mask, ~_value_mask);
> 49:     *reinterpret_cast<uint64_t*>(data + offset) = static_cast<uint64_t>(key) | (static_cast<uint64_t>(value) << _value_shift);

How does this line not get a signal for unaligned write?

src/hotspot/share/utilities/packedTable.cpp line 83:

> 81:     assert(mid >= low && mid < high, "integer overflow?");
> 82:     uint64_t element = read_element(data, length, _element_bytes * mid);
> 83:     uint32_t key = element & _key_mask;

All this casting is hard to follow so I added this at the beginning of the file:

#ifndef _WIN32
#pragma GCC diagnostic warning "-Wconversion"
#endif

and this line, 102, and 87 complain:


warning: conversion from 'uint64_t' {aka 'long unsigned int'} to 'uint32_t' {aka 'unsigned int'} may change value [-Wconversion]
   87 |     uint32_t key = element & _key_mask;
      |                    ~~~~~~~~^~~~~~~~~~~


If the value is okay to cast to uint32_t, which I believe it is, use checked_cast<uint32_t> from checkedCast.hpp.

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

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24847#pullrequestreview-2905861595
PR Review Comment: https://git.openjdk.org/jdk/pull/24847#discussion_r2132749485
PR Review Comment: https://git.openjdk.org/jdk/pull/24847#discussion_r2132790669