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

[Andrew Dinn]

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

src/hotspot/share/interpreter/linkResolver.cpp line 1706:

> 1704:     Klass* resolved_klass = link_info.resolved_klass();
> 1705:     methodHandle method(THREAD, method_entry->method());
> 1706:     Handle     appendix(THREAD, pool->cache()->appendix_if_resolved(index));

This is another point where you already have the `ResolvedMethodEntry` and are still looking up the appendix info via the cache using an index.

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

> 242:     if ((*opc) == (u1)Bytecodes::_invokevirtual ||
> 243:         // allow invokespecial as an alias, although it would be very odd:
> 244:         ((*opc) == (u1)Bytecodes::_invokespecial && _pool->tag_at(cp_index).is_method())) {

I'm not clear why the assert for both cases has now been folded into an additional logic check for only one case. You don't seem to have added an else clause to handle the case where the opcode is `_invokespecial` and the pool tag is not a method. Can you explain this change?

src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 2292:

> 2290:         if (entry->has_appendix()) {
> 2291:           constantPoolHandle cp(THREAD, METHOD->constants());
> 2292:           SET_STACK_OBJECT(cp->cache()->appendix_if_resolved(index), 0);

Another place where you indirect through the cache using an index when you already have the entry.

src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 2314:

> 2312:           CALL_VM(InterpreterRuntime::resolve_from_cache(THREAD, (Bytecodes::Code)opcode),
> 2313:                   handle_exception);
> 2314:           entry = cp->resolved_method_entry_at(index);

Do you actually need to lookup the entry again? I'm not really sure why the old code needed to do so.

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 1685:

> 1683:       vmassert(MethodHandles::is_signature_polymorphic_method(resolved_method()),"!");
> 1684:       vmassert(!MethodHandles::is_signature_polymorphic_static(resolved_method->intrinsic_id()), "!");
> 1685:       vmassert(cp->cache()->appendix_if_resolved(index) == nullptr, "!");

Another place where you have the `ResolvedMethodEntry` and are accessing via the cache with an index.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1363399362
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1363415505
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1363423644
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1363427943
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1363440870