RFR: 8301997: Move method resolution information out of the cpCache [v4]

[Coleen Phillimore]

On Mon, 23 Oct 2023 19:53:54 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> The current structure used to store the resolution information for methods, ConstantPoolCacheEntry, is difficult to interpret due to its ambigious fields f1 and f2. This structure previously held information for fields, methods, and invokedynamic calls which were all encoded into f1 and f2. Currently this structure only handles method entries, but it remains obtuse and inconsistent with recent changes. 
>> 
>> This enhancement introduces a new data structure that stores the necessary resolution data in an intuitive an extensible manner. These resolved entries are stored in an array inside the constant pool cache in a very similar manner to invokedynamic entries in JDK-8301995.
>> 
>> Instances of ConstantPoolCache entry related to field resolution have been replaced with the new ResolvedMethodEntry, and ConstantPoolCacheEntry has been removed entirely. The class ResolvedMethodEntry holds resolution information for all types of invoke calls besides invokedynamic, and thus has fields that may be unused depending on the invoke code. 
>> 
>> To streamline the review, please consider these major areas that have been changed:
>> 1. ResolvedMethodEntry class
>> 2. Rewriter for initialization of the structure
>> 3. cpCache for resolution
>> 4. InterpreterRuntime, linkResolver, and templateTable
>> 5. JVMCI
>> 6. SA
>> 
>> Verified with tier 1-9 tests.
>> 
>> This change supports the following platforms: x86, aarch64
>
> Matias Saavedra Silva has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
> 
>  - Merge branch 'master' into method_entry_8301997
>  - Added asserts for getters and fixed printing
>  - Removed dead code in interpreters
>  - Removed unused structures, improved set_method_handle and appendix_if_resolved
>  - Removed some comments and relocated code
>  - 8301997: Move method resolution information out of the cpCache

This is really great work and a wonderful cleanup of all these special cases we've had to hack into this over the years.  My comments are alignment nits, plus not including instanceKlass.hpp if you don't need to.  Really nice.

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2470:

> 2468:   __ load_unsigned_short(method_or_table_index, Address(cache, in_bytes(ResolvedMethodEntry::table_index_offset())));
> 2469:   __ bind(Done);
> 2470: }

I like how you broke this up.  There's a little duplication but I don't see a nice way of doing away with it and this makes it clear which parts of the ResolvedMethodEntry is meaningful for each bytecode.  If someday in the future we wanted to further specialize for invokeinterface for example, your structure makes that easy to do.

src/hotspot/cpu/x86/templateTable_x86.cpp line 3800:

> 3798:                                                rbx, // Method*
> 3799:                                                rdx  // flags
> 3800:                                                );

small nit: can you put ); on the line before?  And in the call below it to load_resolved_method_entry_special_or_static?

src/hotspot/share/interpreter/rewriter.hpp line 88:

> 86:     return _cp_cache_map.length() - _first_iteration_cp_cache_limit;
> 87:   }
> 88: 

I'm so pleased this special case went away.  We still need to add entries for invokespecial of JVM_CONSTANT_InterfaceMethodref but we don't need to keep track of where the original entries ended anymore with your changes.

src/hotspot/share/interpreter/templateTable.hpp line 288:

> 286:                                                Register klass,
> 287:                                                Register method_or_table_index,
> 288:                                                Register flags);

Nit: can you line up these arguments?  You did move these. thank you.

src/hotspot/share/oops/cpCache.cpp line 247:

> 245: 
> 246: void ConstantPoolCache::set_itable_call(Bytecodes::Code invoke_code,
> 247:                                              int method_index,

Nit: align parameters here.

src/hotspot/share/oops/resolvedMethodEntry.cpp line 35:

> 33:       return !_method->is_old() && !_method->is_obsolete(); // old is always set for old and obsolete
> 34:   } else {
> 35:       return true;

nit: indent 2.

src/hotspot/share/oops/resolvedMethodEntry.hpp line 29:

> 27: 
> 28: #include "interpreter/bytecodes.hpp"
> 29: #include "oops/instanceKlass.hpp"

Can you forward declare InstanceKlass rather than including it here?

-------------

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15455#pullrequestreview-1700425315
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1373666390
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1373678584
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1373695735
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1373697254
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1373704477
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1373732369
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1373735034