Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
On Tue, 14 Mar 2023 23:29:17 GMT, Calvin Cheung wrote:
>> Matias Saavedra Silva has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> RISCV port update
>
> 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?
The if statements below use braces so I think it would be better to add braces
to the else case.
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
On Tue, 14 Mar 2023 20:20:41 GMT, Matias Saavedra Silva 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 Changes requested by gcao (Author). - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
On Tue, 14 Mar 2023 20:20:41 GMT, Matias Saavedra Silva
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* 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) (0x &
> 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
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/a3e7ca02..db892223 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12778&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12778&range=02-03 Stats: 23 lines in 2 files changed: 5 ins; 12 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
