On Wed, 10 Sep 2025 16:19:17 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.
Overall this looks good - a lot to digest though, so this is only an initial
pass (and I'm not that familiar with the existing code).
Quite a few minor issues (mainly use of `CHECK` macros), some comments and
suggestions.
Thanks
src/hotspot/share/classfile/classFileParser.cpp line 3327:
> 3325: bootstrap_methods_u2_len,
> 3326: _loader_data,
> 3327: CHECK);
Suggestion:
cp->bsm_entries().start_extension(num_bootstrap_methods,
bootstrap_methods_u2_len,
_loader_data,
CHECK);
Fix indent
src/hotspot/share/classfile/classFileParser.cpp line 3338:
> 3336: "bootstrap_method_index %u has bad constant type in class file
> %s",
> 3337: bootstrap_method_ref,
> 3338: CHECK);
Suggestion:
bootstrap_method_ref,
CHECK);
Fix indent
src/hotspot/share/oops/constantPool.cpp line 53:
> 51: #include "memory/universe.hpp"
> 52: #include "oops/array.hpp"
> 53: #include "oops/constantPool.hpp"
Suggestion:
You don't include the base header if including the .inline.hpp
src/hotspot/share/oops/constantPool.cpp line 1614:
> 1612: ConstantPool::start_extension(const constantPoolHandle& ext_cp, TRAPS) {
> 1613: return bsm_entries().start_extension(ext_cp->bsm_entries(),
> pool_holder()->class_loader_data(),
> 1614:
> CHECK_(BSMAttributeEntries::InsertionIterator()));
You can't use a `CHECK` macro on a return statement - the expansion puts the
exception check after the return where it is unreachable.
src/hotspot/share/oops/constantPool.cpp line 1619:
> 1617:
> 1618: void ConstantPool::end_extension(BSMAttributeEntries::InsertionIterator
> iter, TRAPS) {
> 1619: bsm_entries().end_extension(iter, pool_holder()->class_loader_data(),
> CHECK);
Again no CHECK on a return. In this case all you need is `THREAD`.
src/hotspot/share/oops/constantPool.cpp line 1625:
> 1623: void ConstantPool::copy_bsm_entries(const constantPoolHandle& from_cp,
> 1624: const constantPoolHandle& to_cp,
> 1625: TRAPS) {
Suggestion:
void ConstantPool::copy_bsm_entries(const constantPoolHandle& from_cp,
const constantPoolHandle& to_cp,
TRAPS) {
Fix indent
src/hotspot/share/oops/constantPool.cpp line 1628:
> 1626: to_cp->bsm_entries().append(from_cp->bsm_entries(),
> 1627: to_cp->pool_holder()->class_loader_data(),
> 1628: CHECK);
Suggestion:
to_cp->bsm_entries().append(from_cp->bsm_entries(),
to_cp->pool_holder()->class_loader_data(),
THREAD);
Fix indent, plus (pre-existing) no need for CHECK when you will return anyway.
src/hotspot/share/oops/constantPool.cpp line 1659:
> 1657: }
> 1658: }
> 1659: copy_bsm_entries(from_cp, to_cp, CHECK);
Suggestion:
copy_bsm_entries(from_cp, to_cp, THREAD);
(pre-existing) no need for CHECK when you will return anyway.
src/hotspot/share/oops/constantPool.cpp line 1831:
> 1829: const BSMAttributeEntry* const bsmae2 =
> cp2->bsm_attribute_entry(idx2);
> 1830: int cp_entry_index1 = bsmae1->bootstrap_method_index();
> 1831: int cp_entry_index2 = bsmae2->bootstrap_method_index();
The existing simple names are more readable in my opinion.
src/hotspot/share/oops/constantPool.cpp line 1859:
> 1857: // Return the index of a matching bootstrap attribute record or (-1) if
> there is no match.
> 1858: int ConstantPool::find_matching_bsm_entry(int pattern_i,
> 1859: const constantPoolHandle& search_cp, int
> offset_limit) {
Suggestion:
int ConstantPool::find_matching_bsm_entry(int pattern_i,
const constantPoolHandle& search_cp,
int offset_limit) {
Fix indent
src/hotspot/share/oops/constantPool.cpp line 2348:
> 2346: assert(num_entries + iter._cur_offset <=
> iter.insert_into->_offsets->length(), "must");
> 2347: for (int i = 0; i < num_entries; i++) {
> 2348: const BSMAttributeEntry* bsmae = entry(i);
Nit: It's okay to use a simple name like `e` to represent `entry` - when you
don't have different types of entries involved we don't need to encode the type
in the variable name. EDIT: just like in
`BSMAttributeEntries::InsertionIterator::reserve_new_entry`.
src/hotspot/share/oops/constantPool.cpp line 2355:
> 2353:
> 2354: BSMAttributeEntries::InsertionIterator
> BSMAttributeEntries::start_extension(const BSMAttributeEntries& other,
> ClassLoaderData* loader_data, TRAPS) {
> 2355: return start_extension(other.number_of_entries(),
> other.array_length(), loader_data,
> CHECK_(BSMAttributeEntries::InsertionIterator()));
Again can't use `CHECK_` on a return statement.
src/hotspot/share/oops/constantPool.cpp line 2388:
> 2386: InsertionIterator iter = start_extension(other, loader_data, CHECK);
> 2387: other.copy_into(iter, other.number_of_entries());
> 2388: end_extension(iter, loader_data, CHECK);
Again no need for CHECK on final statement.
src/hotspot/share/oops/constantPool.cpp line 2393:
> 2391:
> 2392: void BSMAttributeEntries::end_extension(InsertionIterator& iter,
> ClassLoaderData* loader_data,
> 2393: TRAPS) {
Suggestion:
void BSMAttributeEntries::end_extension(InsertionIterator& iter,
ClassLoaderData* loader_data, TRAPS) {
This line is still shorter than 2382 above
src/hotspot/share/oops/constantPool.hpp line 145:
> 143: : insert_into(insert_into),
> 144: _cur_offset(cur_offset),
> 145: _cur_array(cur_array) {}
Not sure what the indentation rules are for constructors but I think we need
some more.
src/hotspot/share/oops/constantPool.hpp line 200:
> 198: // Extend to have the space for both this BSMAEntries and other's.
> 199: // Does not copy in the other's BSMAEntrys, that must be done via the
> InsertionIterator.
> 200: // This starts an insertion iterator. Any call to start_extension must
> have a matching end_exntesion call.
Suggestion:
// This starts an insertion iterator. Any call to start_extension must have a
matching end_extension call.
src/hotspot/share/oops/constantPool.hpp line 224:
> 222: InstanceKlass* _pool_holder; // the corresponding class
> 223:
> 224: BSMAttributeEntries _bsmaentries;
Nit: `_entries` would suffice.
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 394:
> 392: write_attribute_name_index("BootstrapMethods");
> 393: u4 length = sizeof(u2) + // num_bootstrap_methods
> 394: // The rest of it
The comment doesn't add any value here.
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 398:
> 396: write_u4(length);
> 397:
> 398: int num_bootstrap_methods = cpool()->bsm_entries().number_of_entries();
I'm confused about the seemingly different uses of `num_bootstrap_methods` here
and in the comment above:
sizeof(u2) + // num_bootstrap_methods
?
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.
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.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstantPool.java line
444:
> 442: a = bsmaentries_offsets.getValue(a);
> 443: return VMObjectFactory.newObject(U4Array.class, a);
> 444: }
Suggestion:
private U4Array getOffsets() {
Address a = getAddress().addOffsetTo(bsmaentries);
if (a == null) return null;
a = bsmaentries_offsets.getValue(a);
return VMObjectFactory.newObject(U4Array.class, a);
}
Fix indent. (Note this file is using 2-indent instead of the 4-indent it
should for Java code.)
-------------
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/27198#pullrequestreview-3227461452
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350904057
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350905434
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350911716
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350919770
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350921555
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350923178
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350926508
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350929316
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350937167
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350939688
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350948252
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350949299
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350952103
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350954308
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350974468
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350977926
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350979954
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350990306
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350994934
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2350996928
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2351002821
PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2351014174
