On Wed, 5 Nov 2025 08:37:22 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. > > Johan Sjölen has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 31 additional > commits since the last revision: > > - Merge remote-tracking branch 'openjdk/master' into operands-again > - It's fine to initialize the iterator with null, it's not fine to reserve > an entry if it's null > - Fix naming > - Serguei comments > - Revert change > - Some nits > - Fix copyright > - Move BSMAttribute BSMAttributeEntries to own header file > - Merge remote-tracking branch 'origin/operands-again' into operands-again > - Apply suggestions from code review > > Co-authored-by: David Holmes > <[email protected]> > - ... and 21 more: https://git.openjdk.org/jdk/compare/edfdae41...57f0093e This is excellent work! I had a couple of small suggested changes, but am happy to approve it. src/hotspot/share/oops/bsmAttribute.hpp line 28: > 26: #define SHARE_OOPS_BSMATTRIBUTE_HPP > 27: > 28: #include "classfile/classLoaderData.hpp" I think you can forward declare ClassLoaderData rather than include the whole file here. ------------- PR Review: https://git.openjdk.org/jdk/pull/27198#pullrequestreview-3421758555 PR Review Comment: https://git.openjdk.org/jdk/pull/27198#discussion_r2494308893
