RFR: 8367656: Refactor Constantpool's operand array into two

[Johan Sjölen]

On Tue, 16 Sep 2025 06:24:25 GMT, David Holmes <dholmes 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.
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 49:
> 
>> 47: #include "oops/annotations.hpp"
>> 48: #include "oops/constantPool.hpp"
>> 49: #include "oops/constantPool.inline.hpp"
> 
> Suggestion:
> 
> #include "oops/constantPool.inline.hpp"
> 
> You don't include the regular header when you include the inline one.

I wasn't aware! Thanks

> src/hotspot/share/prims/jvmtiRedefineClasses.hpp line 371:
> 
>> 369:   intArray *                  _bsm_index_map_p;
>> 370: 
>> 371:   // After merge_constant_pools pass 0 the BSMAttribute entries of merge_cp_p will have been expanded to fit scratch_cp.
> 
> This comment doesn't read correctly.

There's a missing comma but otherwise it reads fine to me? I added a few more words. There are multiple passes in the method `merge_constant_pools` and they are numbered.


  // After merge_constant_pools pass 0, the BSMAttribute entries of merge_cp_p will have been expanded to fit
  // scratch_cp's BSMAttribute entries as well.
  // However, the newly acquired space will not have been filled in yet.
  // To append to this new space, the iterator is used.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2351203555
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2351202052