RFR: 8301997: Move method resolution information out of the cpCache

[Matias Saavedra Silva]

On Tue, 17 Oct 2023 10:21:40 GMT, Andrew Dinn <adinn 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
>
> src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2261:
> 
>> 2259: // volatile-loads.
>> 2260: 
>> 2261: void TemplateTable::resolve_cache_and_index_for_field(int byte_no,
> 
> This change looks more confusing than it actually needs because you deleted the definition for `resolve_cache_and_index` that preceded `resolve_cache_and_index_for_field` and then added `resolve_cache_and_index_for_method` after `resolve_cache_and_index_for_field`. As a result we see changes made to `resolve_cache_and_index` that re-introduce changes already made for `resolve_cache_and_index_for_field` followed by changes to `resolve_cache_and_index_for_field` that rework it to remove field-specific operations and replace them with method-specific operations.
> 
> That's going to make it a lot harder for maintainers and back-porters to work out what this change is really doing. If instead you place the definition for `resolve_cache_and_index_for_method` before the definition for `resolve_cache_and_index_for_field` then the diff will show `resolve_cache_and_index` being repurposed to cater for methods and the definition of `resolve_cache_and_index_for_field` should remain unchanged.

Great point! I didn't notice how messy the diff ended up looking.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1362462387