RFR: 8312235: [JVMCI] ConstantPool should not force eager resolution [v2]

[Matias Saavedra Silva]

On Thu, 20 Jul 2023 18:05:07 GMT, Doug Simon <dnsimon at openjdk.org> wrote:

>> The existing `jdk.vm.ci.meta.ConstantPool.lookupConstant(int cpi)` method forces eager resolving of constants. For `DynamicConstant`, `MethodHandle` and `MethodType`, this can mean invoking bootstrap methods, something that should not be done during JIT compilation. To avoid this, this PR adds the following to `jdk.vm.ci.meta.ConstantPool`:
>> 
>> 
>> /**
>>  * Looks up a constant at the specified index.
>>  *
>>  * If {@code resolve == false} and the denoted constant is of type
>>  * {@code JVM_CONSTANT_Dynamic}, {@code JVM_CONSTANT_MethodHandle} or
>>  * {@code JVM_CONSTANT_MethodType} and it's not yet resolved then
>>  * {@code null} is returned.
>>  *
>>  * @param cpi the constant pool index
>>  * @return the {@code Constant} or {@code JavaType} instance representing the constant pool
>>  *         entry
>>  */
>> Object lookupConstant(int cpi, boolean resolve);
>> 
>> 
>> Likewise, `jdk.vm.ci.meta.ConstantPool.lookupBootstrapMethodInvocation` has been fixed to no longer invoke the associated bootstrap method.
>> 
>> ---------
>> ### Progress
>> - [ ] Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer))
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>> 
>> 
>> 
>> ### Reviewing
>> <details><summary>Using <code>git</code></summary>
>> 
>> Checkout this PR locally: \
>> `$ git fetch https://git.openjdk.org/jdk.git pull/14927/head:pull/14927` \
>> `$ git checkout pull/14927`
>> 
>> Update a local copy of the PR: \
>> `$ git checkout pull/14927` \
>> `$ git pull https://git.openjdk.org/jdk.git pull/14927/head`
>> 
>> </details>
>> <details><summary>Using Skara CLI tools</summary>
>> 
>> Checkout this PR locally: \
>> `$ git pr checkout 14927`
>> 
>> View PR using the GUI difftool: \
>> `$ git pr show -t 14927`
>> 
>> </details>
>> <details><summary>Using diff file</summary>
>> 
>> Download this PR as a diff file: \
>> <a href="https://git.openjdk.org/jdk/pull/14927.diff">https://git.openjdk.org/jdk/pull/14927.diff</a>
>> 
>> </details>
>
> Doug Simon has updated the pull request incrementally with one additional commit since the last revision:
> 
>   avoid bootstrap method invocation by lookupBootstrapMethodInfo

This looks good! I just have a few small questions and concerns.

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

> 698: C2V_END
> 699: 
> 700: C2V_VMENTRY_NULL(jobject, lookupConstantInPool, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint index, bool resolve))

Maybe rename `jint index` to `cpi`, `cp_index`, or something related. I am currently making an effort to replace `index` with something more specific when used in relation to the Constant Pool, and I think it's best to exercise that here as well.

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

> 705:     obj = cp->find_cached_constant_at(index, found_it, CHECK_NULL);
> 706:     if (!found_it) {
> 707:       return nullptr;

Should there be some sort of exception or error checking here or in the caller? If the constant can't be found it either isn't resolved or the index is invalid, correct?

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

> 1587: C2V_END
> 1588: 
> 1589: C2V_VMENTRY_0(int, invokeDynamicOperandToCPIndex, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint operand, jboolean resolve))

Is `invokeDynamicOperand` the correct name here? From what I recall, the operand to an `invokedynamic` instruction is a constant pool index which is later rewritten to an `indy_index`. I think the term operand can be confusing here. 

Given the way that this method works, the operand in question is always an encoded `indy_index`.

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

PR Review: https://git.openjdk.org/jdk/pull/14927#pullrequestreview-1547792940
PR Review Comment: https://git.openjdk.org/jdk/pull/14927#discussion_r1274997653
PR Review Comment: https://git.openjdk.org/jdk/pull/14927#discussion_r1274993497
PR Review Comment: https://git.openjdk.org/jdk/pull/14927#discussion_r1275004183