-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8315771: [JVMCI] Resolution of bootstrap methods with int[] static arguments #15588
Conversation
👋 Welcome back Zeavee! A progress list of the required criteria for merging this PR into |
Webrevs
|
@@ -170,6 +170,17 @@ interface BootstrapMethodInvocation { | |||
List<JavaConstant> getStaticArguments(); | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very confusing documentation. It's not clear what index or entry you are referring to each time you say "index". Please provide a concrete example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rewritten the documentation, I hope it is clearer. I also added a second argument to be able to access different different static arguments on the same bootstrap specifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still needs an example showing how this API should be used. Please add a complete (pseudo-code) example to the javadoc of BootstrapMethodInvocation
.
…ccess different static arguments
/** | ||
* Gets the constant pool index of a static argument of a bootstrap specifier. Used when the list | ||
* of static arguments in the {@link BootstrapMethodInvocation} is an {@code int[]}. The list has | ||
* two elements. The first one is the number of arguments and the second one is the {@code cpi}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to BootstrapMethodInvocation.getStaticArguments()
whose return type is List<JavaConstant>
, not int[]
?
The list has two elements.
That's not consistent with the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to BootstrapMethodInvocation.getStaticArguments() whose return type is List, not int[]?
It is indeed the list returned by BootstrapMethodInvocation.getStaticArguments()
. In this case, we have in fact a List which corresponds to the int[], but it is true it is confusing, I will rephrase it.
That's not consistent with the code.
In this case, the arguments come from here, which are always only two elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case
In what case? When introducing new API like this, you need to use precise language that spells out possible values, assumptions, typical usages etc. What you have written is a good start but needs more work.
* of static arguments in the {@link BootstrapMethodInvocation} is an {@code int[]}. The list has | ||
* two elements. The first one is the number of arguments and the second one is the {@code cpi}. | ||
* | ||
* @param cpi the index of a bootstrap specifier in the constant pool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the index of a bootstrap specifier in the constant pool
What's a bootstrap specifier? That is not mentioned anywhere else so you either have to explain it or use existing terminology.
@@ -170,6 +170,17 @@ interface BootstrapMethodInvocation { | |||
List<JavaConstant> getStaticArguments(); | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still needs an example showing how this API should be used. Please add a complete (pseudo-code) example to the javadoc of BootstrapMethodInvocation
.
…or BootstrapMethodInvocation usage
* {@link BootstrapMethodInvocation} is a {@code List<PrimitiveConstant>} of the form | ||
* {{@code arg_count}, {@code pool_index}}, meaning the arguments are not already resolved and that | ||
* the JDK has to lookup the arguments when they are needed. The {@code cpi} corresponds to | ||
* {@code pool_index} and the {@code index} has to be smaller than {@code arg_count}. | ||
* | ||
* @param cpi the index of a bootstrap specifier in the constant pool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param cpi the index of a {@code CONSTANT_Dynamic_info} or @{code CONSTANT_InvokeDynamic_info} entry
* argCount = staticArguments.get(0).asInt(); | ||
* cpi = staticArguments.get(1).asInt(); | ||
* for (int i = 0; i < argCount; ++i) { | ||
* arguments[i] = lookupConstant(cpi, i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What lookupConstant
method is this?
Where is the usage of the new bootstrapArgumentIndexAt
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a mistake sorry, it is now correct.
The idea is to use bootstrapArgumentIndexAt
to obtain the constant pool index of the static argument and use it to call ConstantPool.lookupConstant(int cpi, boolean resolve)
to get the constant.
* {@code pool_index} and the {@code index} has to be smaller than {@code arg_count}. | ||
* | ||
* @param cpi the index of a {@code CONSTANT_Dynamic_info} or @{code CONSTANT_InvokeDynamic_info} entry | ||
* @param index the index of the static argument in the list of static arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when index
is out of bounds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, it returns another constant pool index or a random number. I can add a check in the CachedBSMArgs.get
method to prevent this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please. We should opt for exceptions over undefined behavior whenever possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After double checking, there is in fact already an assert
for checking if the index is out of bounds. Should I change it to an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the assertion in code that can throw exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion is here. I don't think we can throw exception there. I could check the size in jvmciCompilerToVM.bootstrapArgumentIndexAt
, but it would require a bit of code duplication to obtain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. You should just add a note in the javadoc that the result is undefined if index
is out of bounds. This is all internal API so I assume we can guarantee the value is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is only used in CachedBSMArgs.get
, which will throw an ArrayIndexOutOfBoundsException
before we arrive there.
@@ -165,6 +165,23 @@ interface BootstrapMethodInvocation { | |||
/** | |||
* Gets the static arguments with which the bootstrap method will be invoked. | |||
* | |||
* An argument of type {@link PrimitiveConstant} represents a {@code CONSTANT_Dynamic_info} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it not be an index to any valid LDC constant? This is implied by this code in your Graal PR:
if (constant instanceof PrimitiveConstant primitiveConstant) {
int argCpi = primitiveConstant.asInt();
Object argConstant = lookupConstant(argCpi, opcode == Opcodes.INVOKEDYNAMIC ? Opcodes.LDC : opcode, allowBootstrapMethodInvocation);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, sorry, I will fix this documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the user point of view, a PrimitiveConstant
can in fact only be a ConstantDynamic
, because the other type of constant are automatically resolved in CachedBSMArgs.get
. In the Graal PR, lookupConstant
is a recursive call, allowing to call the associated bootstrap method, but the other type of entries will go in the else
branch.
@@ -526,6 +529,37 @@ private int flags() { | |||
return UNSAFE.getInt(getConstantPoolPointer() + config().constantPoolFlagsOffset); | |||
} | |||
|
|||
static class CachedBSMArgs extends AbstractList<JavaConstant> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class needs documentation.
@PaulSandoz it would be great if you could have a look at this PR or suggest another domain expert who might be willing to look at it. |
@Zeavee please keep the PR description up to date with the current changes. |
…d documentation for CachedBSMArgs
@dougxc i will look at it this week |
* {@code pool_index} and the {@code index} has to be smaller than {@code arg_count}. | ||
* | ||
* @param cpi the index of a {@code CONSTANT_Dynamic_info} or @{code CONSTANT_InvokeDynamic_info} entry | ||
* @param index the index of the static argument in the list of static arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please. We should opt for exceptions over undefined behavior whenever possible.
* @return A {@link PrimitiveConstant} representing a {@code CONSTANT_Dynamic_info} | ||
* entry or a {@link JavaConstant} representing the static argument requested | ||
* @return A {@link PrimitiveConstant} representing an unresolved constant pool entry | ||
* or a {@link JavaConstant} representing the static argument requested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or a {@link JavaConstant}
a PrimitiveConstant is a JavaConstant isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a PrimitiveConstant is a JavaConstant isn't it?
Yes, it is, but since the primitives are returned in their boxed form, a PrimitiveConstant can only be a constant pool index, so I think it is better to specify the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that a PrimitiveConstant is a subtype of JavaConstant so the "or" does not make sense. It's like saying "an int or a number".
} else if (object instanceof JavaType type) { | ||
res = runtime().getReflection().forObject(type); | ||
} else { | ||
res = JavaConstant.forInt(argCpi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my understanding and my testing, this can only be reached for CONSTANT_Dynamic
constant pool entries, as other types of entries should resolve even with resolve = false
. Am I correct or am I missing some edge cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's right based on the spec for ConstantPool.lookupConstant
.
… update documentation for @return of CachedBSMArgs.get
Please update |
* entry. To resolve this entry, the corresponding bootstrap method has to be called first: | ||
* | ||
* <pre> | ||
* lookupConstant(int index, boolean resolve) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example should focus on resolving the static arguments:
List<JavaConstant> args = cp.getStaticArguments();
List<JavaConstant> resolvedArgs = new ArrayList<>(args.size());
for (JavaConstant c : args) {
JavaConstant r = c;
if (c instanceof PrimitiveConstant pc) {
r = ...
}
resolvedArgs.append(rc);
}
It should also use real Java code to remove all ambiguity.
…cConstant to test the new functionality
Some general comments:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now.
@Zeavee 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:
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 232 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dougxc, @PaulSandoz) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
This PR still needs another review given that it's not trivial. Maybe @TobiHartmann or @tkrodriguez can help out. |
This generally looks good to me. I was initially confused about the reuse (or overload?) of It's very subtle, and I wondering if using a new type would be better and more clearly distinguish between a primitive constant and an index in the constant pool. However, i am not an expert in JVMCI so do not know what repercussions that might have. It's as if you want to expose a lazily resolved constant. |
While it's tempting to introduce a new type to distinguish between a primitive constant and an index in the constant pool, it would involve changing API that is already used in Graal. @Zeavee can you please update the javadoc of
No need to re-run testing after this change as it's only a doc change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I suppose you could generalize the test a little more to iterate over the static arguments, independent of the condyType. That exercises the code a little more esp. when running under the -XX:UseBootstrapCallInfo=3
argument. Up to you.
/integrate |
/sponsor |
Going to push as commit 015f6f5.
Your commit was automatically rebased without conflicts. |
@@ -32,6 +32,9 @@ | |||
* @run testng/othervm | |||
* -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:-UseJVMCICompiler | |||
* jdk.vm.ci.hotspot.test.TestDynamicConstant | |||
* @run testng/othervm | |||
* -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:-UseJVMCICompiler -XX:UseBootstrapCallInfo=3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: VM option 'UseBootstrapCallInfo' is diagnostic and must be enabled via -XX:+UnlockDiagnosticVMOptions.
New bug being filed.
Currently,
jdk.vm.ci.meta.ConstantPool.lookupBootstrapMethodInvocation
does not support static arguments of typeint[]
.Supporting those static arguments allows to correctly lookup the
BootstrapMethodInvocation
of someInvokeDynamic
andDynamicConstant
.To lookup the constant at the index in the static arguments index list, a new class is introduced, allowing to lazily resolve the constant or obtain the constant pool index of the arguments:
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15588/head:pull/15588
$ git checkout pull/15588
Update a local copy of the PR:
$ git checkout pull/15588
$ git pull https://git.openjdk.org/jdk.git pull/15588/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15588
View PR using the GUI difftool:
$ git pr show -t 15588
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15588.diff
Webrev
Link to Webrev Comment