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

Frederic Parain fparain at openjdk.org
Thu Oct 19 14:22:22 UTC 2023 

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/cpu/x86/templateTable_x86.cpp line 3799:

> 3797:   load_resolved_method_entry_special_or_static(rcx, // ResolvedMethodEntry*
> 3798:                                                rbx, // Method*
> 3799:                                                rdx // flags

Please align the last comment with the comments above.

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

> 3817:   load_resolved_method_entry_special_or_static(rcx, // ResolvedMethodEntry*
> 3818:                                                rbx, // Method*
> 3819:                                                rdx // flags

Please align the last comment with the comments above.

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

> 3842:                                        rax, // Klass*
> 3843:                                        rbx, // Method* or itable/vtable index
> 3844:                                        rdx); // flags

Could all comments be aligned?

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

> 333: }
> 334: 
> 335: void ConstantPoolCache::set_method_handle_common(int method_index,

The only place where this method is called is in set_method_handle() just above. Do we really need to have two methods then?

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

> 348:   ResolvedMethodEntry* method_entry = resolved_method_entry_at(method_index);
> 349: 
> 350:   if (method_entry->is_resolved(invoke_code)) { //method_entry->method() != nullptr &&

Weird comment.

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

> 367:   // In the general case, this could be the call site's MethodType,
> 368:   // for use with java.lang.Invokers.checkExactType, or else a CallSite object.
> 369:   // f1 contains the adapter method which manages the actual call.

Obsolete reference to f1

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

> 376:   //
> 377:   // This means that given a call site like (List)mh.invoke("foo"),
> 378:   // the f1 method has signature '(Ljl/Object;Ljl/invoke/MethodType;)Ljl/Object;',

Obsolete reference to f1

src/hotspot/share/oops/cpCache.hpp line 47:

> 45: 
> 46: // A constant pool cache is a runtime data structure set aside to a constant pool. The cache
> 47: // holds interpreter runtime information for all field access and invoke bytecodes. The cache

The cpCache is not a cache (this name should be changed at some point) and is not interpreter specific.
The cpCache stores the results of fields and methods resolutions, which are then used by all VM components (interpreter, runtime, JITs).

src/hotspot/share/oops/resolvedMethodEntry.hpp line 38:

> 36: // with the constant pool index associated with the bytecode before any resolution is done,
> 37: // where "resolution" refers to populating the bytecode1 and bytecode2 fields and other
> 38: // relevant information.These entries are contained within the ConstantPoolCache and are

Space after the dot.

src/hotspot/share/oops/resolvedMethodEntry.hpp line 45:

> 43: // This structure has fields for every type of invoke bytecode but each entry may only
> 44: // use some of the fields. All entries have a TOS state, number of parameters, flags,
> 45: // and a constant pool index.

An entry can have only one type, and some fields are specific to a particular type of entry.
Would it be possible to use an union for those fields, in order to reduce the size of ResolvedMethodEntry instances?
For instance:


class ResolvedMethodEntry {
  friend class VMStructs;

  Method* _method;                 // Method for non virtual calls, adapter method for invokevirtual, final method for invokevirtual, final method for virtual
  union _specific {
    u2 _resolved_references_index;   // Index of resolved references array that holds the appendix oop for invokehandle
    u2 _table_index;                 // vtable/itable index for virtual and interface calls
    InstanceKlass* _interface_klass; // for interface and static
  } specific;
  u2 _cpool_index;                 // Constant pool index
  u2 _number_of_parameters;        // Number of arguments for method
  u1 _tos_state;                   // TOS state
  u1 _flags;                       // Flags: [000|has_local_signature|has_appendix|forced_virtual|final|virtual_final]
  u1 _bytecode1, _bytecode2;       // Bytecodes for f1 and f2
};

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/BytecodeWithCPIndex.java line 64:

> 62:         return cpCache.getMethodEntryAt(cpCacheIndex).getConstantPoolIndex();
> 63:      }
> 64:      //return cpCache.getEntryAt((int) (0xFFFF & cpCacheIndex)).getConstantPoolIndex();

The commented line should be removed.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ConstantPool.java line 240:

> 238:     } else {
> 239:       // change byte-ordering and go via cache
> 240:       //i = cache.getEntryAt(0xFFFF & which).getConstantPoolIndex();

Commented line should be removed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1365532555
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1365533518
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1365534194
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1364075591
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1364075903
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1364082255
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1364082532
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1364033456
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1364132756
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1365595822
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1335937597
PR Review Comment: https://git.openjdk.org/jdk/pull/15455#discussion_r1335939147

More information about the hotspot-dev mailing list