On Wed, 8 Oct 2025 07:43:01 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 copyright
Changes requested by sspitsyn (Reviewer).
src/hotspot/share/classfile/classFileParser.cpp line 3342:
> 3340:
> 3341: BSMAttributeEntry* entry = iter.reserve_new_entry(bootstrap_method_ref, num_bootstrap_arguments);
> 3342: guarantee_property(entry != nullptr, "Invalid BootstrapMethods num_bootstrap_methods. The total amount of space reserved for the BootstrapMethod attribute was not sufficient", CHECK);
Nit: This line is too big. It is a good idea to split the message. Also, would it better to move this guaranty for `nullptr` into the `reserve_new_entry()`?
src/hotspot/share/classfile/classFileParser.cpp line 3345:
> 3343:
> 3344: for (int argi = 0; argi < num_bootstrap_arguments; argi++) {
> 3345: const u2 bootstrap_argument_index = cfs->get_u2_fast();
Nit: There is no need to rename `argument_index` to `bootstrap_argument_index`. It makes it harder to review. In this specific context it is clear that the argument index is a bootstrap method argument index.
src/hotspot/share/oops/bsmAttribute.hpp line 97:
> 95: int _cur_offset;
> 96: // Current unused offset into BSMAEs bsm-data array
> 97: int _cur_array;
Nit: The declarations at lines 95, 97 will be more readable if comments above declarations trail declarations. The comment should not start with capital.
src/hotspot/share/oops/bsmAttribute.hpp line 120:
> 118: Array<u2>* _bootstrap_methods;
> 119:
> 120: // Copy the first num_entries into iter
Nit: Dot is absent at the end of comment.
src/hotspot/share/oops/bsmAttribute.inline.hpp line 34:
> 32: _cur_array + BSMAttributeEntry::u2s_required(argc) > insert_into->bootstrap_methods()->length()) {
> 33: return nullptr;
> 34: }
Nit: This check needs a comment. Also, I'd suggest to add a guarantee here instead of returning `nullptr`.
src/hotspot/share/oops/bsmAttribute.inline.hpp line 35:
> 33: return nullptr;
> 34: }
> 35: insert_into->_offsets->at_put(_cur_offset, _cur_array);
Q: Can `insert_into->_offsets` be `nullptr` here? Should we add an assert?
src/hotspot/share/oops/constantPool.cpp line 1626:
> 1624: void ConstantPool::copy_bsm_entries(const constantPoolHandle& from_cp,
> 1625: const constantPoolHandle& to_cp,
> 1626: TRAPS) {
Nit: Indent is not right.
src/hotspot/share/oops/constantPool.hpp line 94:
> 92: InstanceKlass* _pool_holder; // the corresponding class
> 93:
> 94: BSMAttributeEntries _bsmaentries;
Nit: Suggestion to rename: `_bsmaentries` => `_bsm_entries`.
src/hotspot/share/oops/constantPool.inline.hpp line 90:
> 88: return resolved_references()->obj_at(cache()->resolved_method_entry_at(index)->resolved_references_index());
> 89: }
> 90:
Nit: Is this really needed?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 682:
> 680: const int new_bs_i = _bsmae_iter.current_offset();
> 681: BSMAttributeEntry* new_bsme =
> 682: _bsmae_iter.reserve_new_entry(new_ref_i, old_bsme->argument_count());
Nit: There is not check for `new_bsme == nullptr`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/27198#pullrequestreview-3316437973
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2414977385
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415001187
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415015413
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415020512
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415042398
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415048756
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415138981
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415149935
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415151472
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2415039223