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

[Coleen Phillimore]

On Tue, 17 Oct 2023 17:45:41 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 incrementally with one additional commit since the last revision:
> 
>   Removed some comments and relocated code

I'm about halfway through.

src/hotspot/cpu/x86/interp_masm_x86.hpp line 311:

> 309:   void load_resolved_indy_entry(Register cache, Register index);
> 310:   void load_field_entry(Register cache, Register index, int bcp_offset = 1);
> 311:   void load_method_entry(Register cache, Register index, int bcp_offset = 1);

As a future RFE, maybe we could put some of these common functions in src/share/interpreter/interp_masm.hpp.  I think you need to remove the declarations for the 4 functions you removed from this and the aarch64 version.

src/hotspot/share/interpreter/interpreterRuntime.cpp line 867:

> 865: 
> 866:   // check if link resolution caused cpCache to be updated
> 867:   if (pool->cache()->resolved_method_entry_at(method_index)->is_resolved(bytecode)) return;

Maybe you could have a local variable to save some characters.

ConstantPoolCache* cache = pool->cache();

src/hotspot/share/interpreter/rewriter.cpp line 228:

> 226:   if (!reverse) {
> 227:     int cp_index = Bytes::get_Java_u2(p);
> 228:     _initialized_method_entries.push(ResolvedMethodEntry((u2)cp_index));

You only need to do this if the constant pool is a JVM_CONSTANT_InterfaceMethodref.  It looks like you're doing this unconditionally.

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

> 43:   GrowableArray<int>  _cp_map;
> 44:   GrowableArray<int>  _cpi_to_method_index_map;
> 45:   GrowableArray<int>  _cp_cache_map;  // for Methodref, Fieldref,

I think this field and a couple other functions in rewriter.hpp still need to be deleted.

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

> 294:                                                  Register method_or_table_index,
> 295:                                                  Register flags);
> 296:   static void load_invoke_cp_cache_entry(int byte_no,

I think there are some functions that need to be removed from TemplateTable now.

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

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15455#pullrequestreview-1683218490
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1362664153
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1362707802
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1362711379
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1362722657
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1362669795