RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry

[Coleen Phillimore]

On Mon, 27 Feb 2023 21:37:34 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

> The current structure used to store the resolution information for invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its ambigious fields f1 and f2. This structure can hold information for fields, methods, and invokedynamics and each of its fields can hold different types of values depending on the entry. 
> 
> This enhancement proposes a new structure to exclusively contain invokedynamic information in a manner that is easy to interpret and easy to extend.  Resolved invokedynamic entries will be stored in an array in the constant pool cache and the operand of the invokedynamic bytecode will be rewritten to be the index into this array.
> 
> Any areas that previously accessed invokedynamic data from ConstantPoolCacheEntry will be replaced with accesses to this new array and structure. Verified with tier1-9 tests.
> 
> The PPC was provided by @reinrich and the RISCV port was provided by @DingliZhang and @zifeihan.
> 
> This change supports the following platforms: x86, aarch64, PPC, and RISCV

This looks really good.  I noted a few changes and questions.

src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 53:

> 51: 
> 52: #undef __
> 53: #define __ Disassembler::hook<InterpreterMacroAssembler>(__FILE__, __LINE__, _masm)->

What is this?  Is this something useful for debugging the template interpreter?  Probably doesn't belong with this change but might be nice to have (?) @reinrich

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

> 2799:                                                bool is_invokevirtual,
> 2800:                                                bool is_invokevfinal, /*unused*/
> 2801:                                                bool is_invokedynamic /*unused*/) {

I assume you have to keep this parameter for the platform that doesn't still have this change (s390)?

src/hotspot/share/cds/classListParser.cpp line 590:

> 588:           // resolve it
> 589:           Handle recv;
> 590:           LinkResolver::resolve_invoke(info, recv, pool, ConstantPool::encode_invokedynamic_index(indy_index), Bytecodes::_invokedynamic, CHECK);

nit: can you reformat so the line isn't so long?

src/hotspot/share/ci/ciReplay.cpp line 419:

> 417:       be used to avoid multiple blocks of similar code. When CPCE is obsoleted
> 418:       these can be removed
> 419:       */

I don't know if you really need this comment.  If so, use // style instead.

src/hotspot/share/ci/ciReplay.cpp line 453:

> 451:         if (!parse_terminator()) {
> 452:           report_error("no dynamic invoke found");
> 453:           return NULL;

nullptr not NULL.

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

> 141:     assert(ref_index >= _resolved_reference_limit, "");
> 142:     if (_pool->tag_at(cp_index).value() != JVM_CONSTANT_InvokeDynamic) {
> 143:       _invokedynamic_references_map.at_put_grow(ref_index, cache_index, -1);

I think you might need to rename _invokedynamic_references_map variable name to _invokehandle_references_map with this change also.  This will be confusng.

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

> 637:   int indy_index = -1;
> 638:   for (int i = 0; i < cp->resolved_indy_entries_length(); i++) {
> 639:     tty->print_cr("Index: %d", cp->resolved_indy_entry_at(i)->constant_pool_index());

Looks like some debugging left in.

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

> 1527:   if (cp_cache_entry->is_resolved(Bytecodes::_invokedynamic)) {
> 1528:     return Bytecodes::_invokedynamic;
> 1529:   }

This seems like it should be removed?

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

> 725:   set_reference_map(nullptr);
> 726: #if INCLUDE_CDS
> 727:   if (_initial_entries != nullptr) {

@iklam with moving invokedynamic entries out, do you still need to save initialized entries ?  Does invokehandle need this? (Should have separate RFE if more cleanup is possible)

src/hotspot/share/oops/resolvedIndyEntry.hpp line 26:

> 24: 
> 25: #ifndef SHARE_OOPS_ResolvedIndyEntry_HPP
> 26: #define SHARE_OOPS_ResolvedIndyEntry_HPP

Make this all capital letters

src/hotspot/share/oops/resolvedIndyEntry.hpp line 71:

> 69: 
> 70:   // Bit shift to get flags
> 71:   // Note: Only one flag exists at the moment but more could be added

Actually two flags - resolution_failed too.

src/hotspot/share/oops/resolvedIndyEntry.hpp line 87:

> 85:   bool is_vfinal()               const { return false;                                            }
> 86:   bool is_final()                const { return false;                                            }
> 87:   bool has_local_signature()     const { return true;                                             }

The closed } don't need to be aligned.

src/hotspot/share/oops/resolvedIndyEntry.hpp line 111:

> 109:     _return_type = return_type;
> 110:     set_flags(has_appendix);
> 111:     Atomic::release_store(&_method, m);

Add a comment like // set this last.  The method is read lock free from the entry and if set, indicates the rest of the resolution information is valid.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyArray.java line 35:

> 33: import sun.jvm.hotspot.types.WrongTypeException;
> 34: import sun.jvm.hotspot.utilities.GenericArray;
> 35: import sun.jvm.hotspot.utilities.Observable;

Do you need all of these imports ?

src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java line 935:

> 933:                 /*if (isInvokedynamicIndex(cpi)) {
> 934:                     compilerToVM().resolveInvokeDynamicInPool(this, cpi);
> 935:                 }*/

Is there something to fix here?

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

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12778