RFR: 8367656: Refactor Constantpool's operand array into two [v5]

David Holmes dholmes at openjdk.org
Fri Sep 19 02:48:27 UTC 2025 

On Thu, 18 Sep 2025 07:41:31 GMT, Johan Sjölen <jsjolen at openjdk.org> 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:
> 
>   Fix rename

This looks good. I think it will be easier to understand for newcomers to the code.

One query: have you done any performance measurements for this change? I'm a little concerned that the insertion iterator and the internal array management is now more heavyweight than the old array management.

I don't understand the existing "operand" terminology and it seems even less appropriate now. It would be good to change this, but can be left for future RFE.

A number of nits, mainly indentation.

Thanks

src/hotspot/share/oops/constantPool.cpp line 1784:

> 1782:     int k1 = from_cp->bootstrap_methods_attribute_index(from_i);
> 1783:     int k2 = from_cp->bootstrap_name_and_type_ref_index_at(from_i);
> 1784:     k1 += to_cp->bsm_entries().array_length();  // to_cp might already have operands

Here and below can we replace "operands" with something better? I didn't understand its use initially and now we don't have an operand array it seems even less informative.

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("/operands[%d]", bsm_entries().bootstrap_methods()->length());

Again is "operands" a meaningful description?

src/hotspot/share/oops/constantPool.cpp line 2358:

> 2356:   InsertionIterator iter = start_extension(other.number_of_entries(), other.array_length(), loader_data, CHECK_(BSMAttributeEntries::InsertionIterator()));
> 2357:   return iter;
> 2358: }

Suggestion:

BSMAttributeEntries::InsertionIterator
BSMAttributeEntries::start_extension(const BSMAttributeEntries& other, ClassLoaderData* loader_data, TRAPS) {
  InsertionIterator iter = start_extension(other.number_of_entries(), other.array_length(), 
                                            loader_data, CHECK_(BSMAttributeEntries::InsertionIterator()));
  return iter;
}

src/hotspot/share/oops/constantPool.cpp line 2362:

> 2360: BSMAttributeEntries::InsertionIterator
> 2361: BSMAttributeEntries::start_extension(int number_of_entries, int array_length,
> 2362:                                    ClassLoaderData* loader_data, TRAPS) {

Suggestion:

                                     ClassLoaderData* loader_data, TRAPS) {

Fix indent

src/hotspot/share/oops/constantPool.cpp line 2391:

> 2389:   other.copy_into(iter, other.number_of_entries());
> 2390:   end_extension(iter, loader_data, THREAD);
> 2391: 

Suggestion:

src/hotspot/share/oops/constantPool.cpp line 2399:

> 2397:   assert(iter._cur_array <= this->_bootstrap_methods->length(), "must be");
> 2398: 
> 2399:   // Did we fill up all of the available space? If so, do nothing

Suggestion:

  // Did we fill up all of the available space? If so, do nothing.

src/hotspot/share/oops/constantPool.hpp line 665:

> 663:   bool compare_bootstrap_entry_to(int bsms_attribute_index1, const constantPoolHandle& cp2,
> 664:                                   int bsms_attribute_index2);
> 665:   // Find a BSM entry in search_cp that matches the BSM at bsm_attribute_index

Suggestion:

  // Find a BSM entry in search_cp that matches the BSM at bsm_attribute_index.

src/hotspot/share/oops/constantPool.hpp line 668:

> 666:   // Return -1 if not found.
> 667:   int find_matching_bsm_entry(int bsms_attribute_index, const constantPoolHandle& search_cp,
> 668:                             int offset_limit);

Suggestion:

  int find_matching_bsm_entry(int bsms_attribute_index, const constantPoolHandle& search_cp,
                              int offset_limit);

Fix indent

src/hotspot/share/oops/constantPool.inline.hpp line 93:

> 91: inline BSMAttributeEntry* BSMAttributeEntries::InsertionIterator::reserve_new_entry(u2 bsmi, u2 argc) {
> 92:   if (_cur_offset + 1 > insert_into->offsets()->length() ||
> 93:       _cur_array + 1 + 1 + argc > insert_into->bootstrap_methods()->length()) {

The `+ 1 + 1 + argc` looks a little magical - can we factor it out and explain it e.g.

int next_array = _cur_array + 1 /* ??? */ + 1 /* ??? */ + argc;
...
_curr_array = next_array;
...

src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 713:

> 711:     // forward reference in *merge_cp_p or not a direct match
> 712:     int found_i = scratch_cp->find_matching_bsm_entry(old_bs_i, *merge_cp_p,
> 713:                                                     max_offset_in_merge);

Suggestion:

    int found_i = scratch_cp->find_matching_bsm_entry(old_bs_i, *merge_cp_p,
                                                      max_offset_in_merge);

Fix indent

src/hotspot/share/prims/jvmtiRedefineClasses.hpp line 451:

> 449:   bool merge_constant_pools(const constantPoolHandle& old_cp,
> 450:     const constantPoolHandle& scratch_cp, constantPoolHandle& merge_cp_p,
> 451:     int& merge_cp_length_p, TRAPS);

Pre-existing but none of the line-wrapping indentation is correct in this section of the file.

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

PR Review: https://git.openjdk.org/jdk/pull/27198#pullrequestreview-3237461571
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361464715
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361479948
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361487875
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361489262
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361209369
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361210519
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361528513
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361225007
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361234056
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361542260
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2361569102

More information about the serviceability-dev mailing list