8312235: [JVMCI] ConstantPool should not force eager resolution #14927

dougxc · 2023-07-18T20:35:54Z

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)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8312235: [JVMCI] need version of ConstantPool.lookupConstant without eager resolution (Bug - P2)

Reviewing

Using git

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14927

View PR using the GUI difftool:
$ git pr show -t 14927

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14927.diff

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8312235: [JVMCI] ConstantPool should not force eager resolution (Bug - P2)

Reviewers

Tom Rodriguez (@tkrodriguez - Reviewer)
Matias Saavedra Silva (@matias9927 - Committer)

Reviewing

Using git

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14927

View PR using the GUI difftool:
$ git pr show -t 14927

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14927.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-07-18T20:36:46Z

👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-07-18T20:37:32Z

@dougxc The following labels will be automatically applied to this pull request:

graal
hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

…thod

mlbridge · 2023-07-19T13:49:53Z

Webrevs

openjdk · 2023-07-20T17:56:07Z

@dougxc Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

tkrodriguez · 2023-07-20T18:08:16Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java

@@ -658,17 +663,18 @@ public Object lookupConstant(int cpi) {
                 * "pseudo strings" (arbitrary live objects) patched into a String entry. Such
                 * entries do not have a symbol in the constant pool slot.
                 */
-                return compilerToVM().resolvePossiblyCachedConstantInPool(this, cpi);
+                return compilerToVM().lookupConstantInPool(this, cpi, true);


This is true because it's always safe to do so?

I don't really know what these "pseudo strings" are. In any case, I don't think resolving them involves calling any Java code.

Sounds good. It's not currently causing any problems and we can investigate further if we start restricting when we can_call_java and problems show up here.

openjdk · 2023-07-21T15:52:41Z

@dougxc This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8312235: [JVMCI] ConstantPool should not force eager resolution

Reviewed-by: never, matsaave

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 42 new commits pushed to the master branch:

7cbab1f: 8312218: Print additional debug information when hitting assert(in_hash)
01e135c: 8312440: assert(cast != nullptr) failed: must have added a cast to pin the node
b7545a6: 8313164: src/java.desktop/windows/native/libawt/windows/awt_Robot.cpp GetRGBPixels adjust releasing of resources
36d578c: 8311653: Modify -XshowSettings launcher behavior
a9d21c6: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
4c2e54f: 8309088: security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java fails
830413f: 8313087: DerValue::toString should output a hex view of the values in byte array
7412193: 4800398: (ch spec) Clarify Channels.newChannel(InputStream) spec
e7726fb: 8313155: Problem list some JUnit-based tests in test/jdk/java/lang/invoke
02a0473: 8312445: Array types in annotation elements show square brackets twice
... and 32 more: https://git.openjdk.org/jdk/compare/0328886450a4978f1edbe06a59d2ab8f733d19a9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

matias9927

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

matias9927 · 2023-07-26T13:53:04Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

+    bool found_it;
+    obj = cp->find_cached_constant_at(index, found_it, CHECK_NULL);
+    if (!found_it) {
+      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?

The error checking is already done by the call to getTagAt in HotSpotConstantPool.lookupConstant(int cpi, boolean resolve).
I'll add more javadoc to clarify when a null is returned.

Note that the javadoc of lookupConstantInPool already states:

* The behavior of this method is undefined if {@code cpi} does not denote one of the following * entry types: {@code JVM_CONSTANT_Dynamic}, {@code JVM_CONSTANT_String}, * {@code JVM_CONSTANT_MethodHandle}, {@code JVM_CONSTANT_MethodHandleInError}, * {@code JVM_CONSTANT_MethodType} and {@code JVM_CONSTANT_MethodTypeInError}.

matias9927 · 2023-07-26T13:56:00Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

@@ -697,9 +697,18 @@ C2V_VMENTRY_NULL(jobject, getUncachedStringInPool, (JNIEnv* env, jobject, ARGUME
  return JVMCIENV->get_jobject(JVMCIENV->get_object_constant(obj));
 C2V_END

-C2V_VMENTRY_NULL(jobject, resolvePossiblyCachedConstantInPool, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint index))
+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.

matias9927 · 2023-07-26T14:00:34Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

-C2V_VMENTRY_0(int, resolveInvokeDynamicInPool, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint index))
-  if (!ConstantPool::is_invokedynamic_index(index)) {
-    JVMCI_THROW_MSG_0(IllegalStateException, err_msg("not an invokedynamic index %d", index));
+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.

I've pushed a commit with better naming.

…pool index

matias9927

Thank you for the changes, approved!

dougxc · 2023-07-27T08:38:45Z

Thanks for the reviews!

/integrate

openjdk · 2023-07-27T08:39:35Z

Going to push as commit 86821a7.
Since your change was applied there have been 42 commits pushed to the master branch:

7cbab1f: 8312218: Print additional debug information when hitting assert(in_hash)
01e135c: 8312440: assert(cast != nullptr) failed: must have added a cast to pin the node
b7545a6: 8313164: src/java.desktop/windows/native/libawt/windows/awt_Robot.cpp GetRGBPixels adjust releasing of resources
36d578c: 8311653: Modify -XshowSettings launcher behavior
a9d21c6: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
4c2e54f: 8309088: security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java fails
830413f: 8313087: DerValue::toString should output a hex view of the values in byte array
7412193: 4800398: (ch spec) Clarify Channels.newChannel(InputStream) spec
e7726fb: 8313155: Problem list some JUnit-based tests in test/jdk/java/lang/invoke
02a0473: 8312445: Array types in annotation elements show square brackets twice
... and 32 more: https://git.openjdk.org/jdk/compare/0328886450a4978f1edbe06a59d2ab8f733d19a9...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-27T08:39:40Z

@dougxc Pushed as commit 86821a7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

add ConstantPool.lookupConstant(int cpi, boolean resolve)

a8e7ead

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jul 18, 2023

update TestDynamicConstant to test new ConstantPool.lookupConstant me…

54c5404

…thod

dougxc marked this pull request as ready for review July 19, 2023 13:44

openjdk bot added the rfr Pull request is ready for review label Jul 19, 2023

avoid bootstrap method invocation by lookupBootstrapMethodInfo

4621c13

dougxc force-pushed the JDK-8312235 branch from 75b0d02 to 4621c13 Compare July 20, 2023 17:55

dougxc changed the title ~~8312235: [JVMCI] need version of ConstantPool.lookupConstant without eager resolution~~ [JVMCI] ConstantPool should not force eager resolution Jul 20, 2023

openjdk bot removed the rfr Pull request is ready for review label Jul 20, 2023

tkrodriguez reviewed Jul 20, 2023

View reviewed changes

tkrodriguez approved these changes Jul 20, 2023

View reviewed changes

dougxc changed the title ~~[JVMCI] ConstantPool should not force eager resolution~~ 8283839: [JVMCI] ConstantPool should not force eager resolution Jul 21, 2023

dougxc changed the title ~~8283839: [JVMCI] ConstantPool should not force eager resolution~~ 8312235: [JVMCI] ConstantPool should not force eager resolution Jul 21, 2023

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 21, 2023

Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8312235

2952db0

matias9927 reviewed Jul 26, 2023

View reviewed changes

dougxc added 3 commits July 26, 2023 17:49

clarified naming around decoding an encoded indy index to a constant …

543d9af

…pool index

clarified when null is returned by lookupConstantInPool

5ada646

renamed index to cp_index

1ca9ab6

matias9927 approved these changes Jul 26, 2023

View reviewed changes

tkrodriguez approved these changes Jul 26, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 27, 2023

openjdk bot closed this Jul 27, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 27, 2023

graalvmbot mentioned this pull request Nov 15, 2023

[GR-45806] Execute bootstrap methods at run time oracle/graal#7834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8312235: [JVMCI] ConstantPool should not force eager resolution #14927

8312235: [JVMCI] ConstantPool should not force eager resolution #14927

dougxc commented Jul 18, 2023 •

edited

Loading

bridgekeeper bot commented Jul 18, 2023

openjdk bot commented Jul 18, 2023

mlbridge bot commented Jul 19, 2023 •

edited

Loading

openjdk bot commented Jul 20, 2023

tkrodriguez Jul 20, 2023

dougxc Jul 20, 2023

tkrodriguez Jul 20, 2023

openjdk bot commented Jul 21, 2023 •

edited

Loading

matias9927 left a comment

matias9927 Jul 26, 2023

dougxc Jul 26, 2023

dougxc Jul 26, 2023

matias9927 Jul 26, 2023

matias9927 Jul 26, 2023

dougxc Jul 26, 2023

matias9927 left a comment

dougxc commented Jul 27, 2023

openjdk bot commented Jul 27, 2023

openjdk bot commented Jul 27, 2023

8312235: [JVMCI] ConstantPool should not force eager resolution #14927

8312235: [JVMCI] ConstantPool should not force eager resolution #14927

Conversation

dougxc commented Jul 18, 2023 • edited Loading

Progress

Issue

Reviewing

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 18, 2023

openjdk bot commented Jul 18, 2023

mlbridge bot commented Jul 19, 2023 • edited Loading

Webrevs

openjdk bot commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jul 21, 2023 • edited Loading

matias9927 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matias9927 left a comment

Choose a reason for hiding this comment

dougxc commented Jul 27, 2023

openjdk bot commented Jul 27, 2023

openjdk bot commented Jul 27, 2023

dougxc commented Jul 18, 2023 •

edited

Loading

mlbridge bot commented Jul 19, 2023 •

edited

Loading

openjdk bot commented Jul 21, 2023 •

edited

Loading