On Mon, 19 May 2025 07:35:16 GMT, Johan Sjölen <jsjo...@openjdk.org> wrote:
> Hi,
>
> The constant pool currently has a lot of methods specific to extracting
> parts of the operands array. What this array actually is, is a sequence of
> bootstrap method attribute entries, where each entry has the following
> components:
>
> ```c++
> struct BSMAE {
> u2 bootstrap_method_index;
> u2 argument_count;
> u2 arguments[argument_count];
> }
>
>
> We can removes some of these operands array specific methods, and instead
> allows you to extract BSMAttributeEntrys which you can then use to extract
> its piece wise components. This makes for a nicer interface, and a bit easier
> to come into as a reader of the code, as it more closely mirrors the JVMS.
>
> Please consider!
>
> Testing: Currently GHA, running tier1-tier3
Overall looks good, a couple of comments below.
src/hotspot/share/oops/constantPool.hpp line 80:
> 78: };
> 79:
> 80: class BSMAttributeEntry {
Please consider moving the description comment from lines #569-#572 ahead of
this structure
src/hotspot/share/oops/constantPool.hpp line 92:
> 90: return reinterpret_cast<u2*>(this + 1);
> 91: }
> 92:
Remove blank lines at #92, #95, #103
src/hotspot/share/oops/constantPool.hpp line 105:
> 103:
> 104: int argument_index(int n) const {
> 105: assert(checked_cast<u2>(n) < _argument_count, "oob");
Should the assert contain a check that _argument_count is >= 0 as well?
src/hotspot/share/oops/constantPool.hpp line 638:
> 636: u2 bootstrap_argument_count_at(int cp_index) {
> 637: assert(tag_at(cp_index).has_bootstrap(), "Corrupted constant pool");
> 638: int bsmai = bootstrap_methods_attribute_index(cp_index);
The "matched ending" assert was the only code that used
bootstrap_operand_limit(), so that method could be removed as well. This
comment applies to bootstrap_operand_base() as well.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstantPool.java line
124:
> 122: private static long elementSize;
> 123:
> 124: private static int INDY_BSM_OFFSET = 0;
Please update the copyright of this file.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25298#pullrequestreview-2858597040
PR Review Comment: https://git.openjdk.org/jdk/pull/25298#discussion_r2100820121
PR Review Comment: https://git.openjdk.org/jdk/pull/25298#discussion_r2100820521
PR Review Comment: https://git.openjdk.org/jdk/pull/25298#discussion_r2100831579
PR Review Comment: https://git.openjdk.org/jdk/pull/25298#discussion_r2100833066
PR Review Comment: https://git.openjdk.org/jdk/pull/25298#discussion_r2100849208
