On Mon, 27 Feb 2023 21:37:34 GMT, Matias Saavedra Silva <[email protected]>
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
