RFR: 8367656: Refactor Constantpool's operand array into two
David Holmes
dholmes at openjdk.org
Tue Sep 16 06:35:44 UTC 2025
On Wed, 10 Sep 2025 16:19:17 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.
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
More information about the serviceability-dev
mailing list