On Tue, 14 Mar 2023 20:20:41 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
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> RISCV port update
Looks good. Just a few minor comments.
src/hotspot/share/interpreter/bootstrapInfo.cpp line 218:
> 216: _indy_index,
> 217:
> pool()->tag_at(_bss_index),
> 218: CHECK_false);
Please indent lines 216-218 like before.
src/hotspot/share/interpreter/bootstrapInfo.cpp line 234:
> 232: if (_indy_index > -1) {
> 233: os::snprintf_checked(what, sizeof(what), "indy#%d", _indy_index);
> 234: }
Since the `else` case doesn’t have braces, maybe omit the braces for this case
as well?
src/hotspot/share/oops/cpCache.cpp line 618:
> 616: indy_resolution_failed(), parameter_size());
> 617: if ((bytecode_1() == Bytecodes::_invokehandle)) {
> 618: constantPoolHandle cph(Thread::current(), cache->constant_pool());
There is another `cph` defined at line 601. Could that one be used?
src/hotspot/share/oops/cpCache.cpp line 652:
> 650: int size = ConstantPoolCache::size(length);
> 651:
> 652: // Initialize resolvedinvokedynamicinfo array with available data
Maybe breakup the long word `resolvedinvokedynamicinfo`?
src/hotspot/share/oops/cpCache.cpp line 653:
> 651:
> 652: // Initialize resolvedinvokedynamicinfo array with available data
> 653: Array<ResolvedIndyEntry>* array;
Suggestion: rename `array` to `resolved_indy_entries`.
src/hotspot/share/oops/cpCache.cpp line 664:
> 662:
> 663: return new (loader_data, size, MetaspaceObj::ConstantPoolCacheType,
> THREAD)
> 664: ConstantPoolCache(length, index_map, invokedynamic_map, array);
I think it reads better if this line is indented to right after the open
parenthesis.
src/hotspot/share/prims/methodComparator.cpp line 119:
> 117: if ((old_cp->name_ref_at(index_old) !=
> new_cp->name_ref_at(index_new)) ||
> 118: (old_cp->signature_ref_at(index_old) !=
> new_cp->signature_ref_at(index_new)))
> 119: return false;
Please adjust the indentations of lines 118 and 119 to be the same as lines 124
and 125.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/BytecodeWithCPIndex.java
line 61:
> 59: } else {
> 60: return cpCache.getEntryAt((int) (0xFFFF &
> cpCacheIndex)).getConstantPoolIndex();
> 61: }
Maybe align all `return` statements with line 56?
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyArray.java
line 38:
> 36: public class ResolvedIndyArray extends GenericArray {
> 37: static {
> 38: VM.registerVMInitializedObserver(new Observer() {
Indentation for java code should be 4 spaces.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyEntry.java
line 38:
> 36: private static long size;
> 37: private static long baseOffset;
> 38: private static CIntegerField cpIndex;
Indentation for java code should be 4 spaces.
-------------
PR: https://git.openjdk.org/jdk/pull/12778
