On Thu, 25 Sep 2025 12:23:11 GMT, Johan Sjölen <[email protected]> wrote:
>> Hi,
>>
>> This is a refactoring of the way that we store the Bootstrap method
>> attribute in the ConstantPool class. We used to have a single `Array<u2>`
>> which was divided into a section of `u4` offsets and a section which was the
>> actual data. In this refactoring we make this split more clear, by actually
>> allocating an `Array<u4>` to store the offsets in and an `Array<u2>` to
>> store the data in. These arrays are then put into a `BSMAttributeEntries`
>> class, which allows us to separate out the API from that of the rest of the
>> `ConstantPool`.
>>
>> We had multiple instances of the code knowing the layout of the operands
>> array and using this to do 'clever' ways of copying and inserting data into
>> it. See `ConstantPool::copy_operands` and `ConstantPool::resize_operands`. I
>> felt like we could do things in a simpler way, so I added the
>> `start_/end_extension` protocol and added the `InsertionIterator` for this.
>> See `ClassFileParser::parse_classfile_bootstrap_methods_attribute` for how
>> this works. I put several relevant definitions into the inline file in hopes
>> of encouraging the compiler to optimize these appropriately.
>>
>> For the Java SA code, I had to add a `U4Array` class. I also had to fix the
>> vmstructs definitions, etc.
>>
>> On the whole, while this code is a bit less terse, I think it's a good API
>> improvement and the underlying implementation of splitting up the operands
>> array is also an improvement.
>>
>> Testing: Oracle Tier1-Tier5 has been run succesfully multiple times. Before
>> integration, I will merge with master and run these tiers again.
>
> Johan Sjölen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Serguei's review
Updates look good. A couple of minor nits spotted.
src/hotspot/share/oops/constantPool.cpp line 761:
> 759: if (!(which >= 0 && which < cpool->resolved_method_entries_length())) {
> 760: // FIXME: should be an assert
> 761: log_debug(class, resolve)("bad bsm %d in:", which); cpool->print();
Suggestion:
log_debug(class, resolve)("bad BSM %d in:", which); cpool->print();
src/hotspot/share/oops/constantPool.cpp line 2303:
> 2301: st->print("constant pool [%d]", length());
> 2302: if (has_preresolution()) st->print("/preresolution");
> 2303: if (!bsm_entries().is_empty()) st->print("/bsms[%d]",
> bsm_entries().bootstrap_methods()->length());
Suggestion:
if (!bsm_entries().is_empty()) st->print("/BSMs[%d]",
bsm_entries().bootstrap_methods()->length());
src/hotspot/share/oops/constantPool.cpp line 2358:
> 2356: BSMAttributeEntries::start_extension(const BSMAttributeEntries& other,
> ClassLoaderData* loader_data, TRAPS) {
> 2357: InsertionIterator iter = start_extension(other.number_of_entries(),
> other.array_length(),
> 2358: loader_data,
> CHECK_(BSMAttributeEntries::InsertionIterator()));
Suggestion:
InsertionIterator iter = start_extension(other.number_of_entries(),
other.array_length(),
loader_data,
CHECK_(BSMAttributeEntries::InsertionIterator()));
-------------
PR Review: https://git.openjdk.org/jdk/pull/27198#pullrequestreview-3277861284
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2386675664
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2386677067
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2386677720
