Constant descriptor resolved too soon during constant resolution?
Jorn Vernee
jbvernee at xs4all.nl
Tue Aug 14 10:59:58 UTC 2018
Thanks!
I had sent some other general code comments to Brian as well, (I thought
he was working on it at that moment), but I should probably just have
sent them here. So here they are again:
These are comments about `DynamicConstantDesc.java`:
- I noticed that in a couple of places the javadoc links to
`DirectMethodHandleDescImpl`, but I think that should be
`DirectMethodHandleDesc` instead? (The interface type, not the
implementation type).
- Also, the javadoc mentions the concept of a "bootstrap method" several
times, but there's no explanation given of what that is exactly. Maybe
that could be included in the javadoc for the class? e.g. what
signatures are valid for bootstrap methods (I recently found out having
`Lookup, String, Class` as first 3 arguments is not actually required).
- Thirdly, I noticed that when calling `withArgs` you often have to
manually specify the type parameter. For instance in
`Enum.EnumDesc#describeConstable`:
```
DynamicConstantDesc.<ConstantDesc<E>>of(BSM_ENUMDESC, CR_EnumDesc)
.withArgs(constantType().descriptorString(),
constantName())
```
I've been running into that myself as well when writing descriptor code.
The fix I have for this in my local copy is to allow `withArgs` to
return a `DynamicConstantDesc` with a different type parameter. i.e.:
```
@Foldable
public <R> DynamicConstantDesc<R> withArgs(ConstantDesc<?>...
bootstrapArgs) {
return DynamicConstantDesc.of(bootstrapMethod, constantName,
constantType, bootstrapArgs);
}
```
Adding the type parameter `R` there. This removes the need to manually
specify the type parameter, and also makes sense semantically since the
return type of the bootstrap method could depend on one of the arguments
(e.g. a generic method who's return type is the type of one of it's
arguments).
- Lastly, as a suggestion for solving "API sharp edge -- varargs version
of DCD.of(...)". Instead of providing an overload for each combination
of arguments you could switch to builder-style methods (which `withArgs`
already kinda is). i.e. adding the methods `withName(String
constantName)` and `withType(ClassDesc constantType)`, and then dropping
the 3 overloads of the `of` method that take a name, type, and name and
type (but no bootstrapArgs). Keeping the one that takes just a bootstrap
method descriptor, and the one that takes 4 arguments (but change that
last one to varargs to allow folding). It's pretty similar, but doesn't
have the overload confusion (right?).
---
Additionally, I wanted to ask if you could add some constants to the
`ConstantDescs` class for working with `LambdaMetafactory`. I have these
going in my local copy:
```
diff -r 3b091f0a5df7
src/java.base/share/classes/java/lang/constant/ConstantDescs.java
--- a/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
Thu Aug 09 22:07:37 2018 +0200
+++ b/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
Tue Aug 14 12:35:50 2018 +0200
@@ -35,6 +35,7 @@
import java.lang.invoke.MethodType;
import java.lang.invoke.VarHandle;
import java.lang.invoke.VarHandle.VarHandleDesc;
+import java.lang.invoke.LambdaMetafactory;
import java.util.Collection;
import java.util.List;
import java.util.Map;
@@ -150,6 +151,10 @@
@Foldable
public static final ClassDesc CR_CallSite =
ClassDesc.of("java.lang.invoke.CallSite");
+ /** {@link ClassDesc} representing {@link LambdaMetafactory} */
+ @Foldable
+ public static final ClassDesc CR_LambdaMetafactory =
ClassDesc.of("java.lang.invoke.LambdaMetafactory");
+
/** {@link ClassDesc} representing {@link Collection} */
@Foldable
public static final ClassDesc CR_Collection =
ClassDesc.of("java.util.Collection");
@@ -264,6 +269,12 @@
= ofConstantBootstrap(CR_ConstantBootstraps, "invoke",
CR_Object, CR_MethodHandle,
CR_Object.arrayType());
+ /** {@link MethodHandleDesc} representing {@link
LambdaMetafactory#metafactory(MethodHandles.Lookup, String, Class,
MethodType, MethodHandle, MethodType)} */
+ @Foldable
+ public static final DirectMethodHandleDesc BSM_METAFACTORY_CONDY
+ = ofConstantBootstrap(CR_LambdaMetafactory, "metafactory",
+ CR_Object, CR_MethodType,
CR_MethodHandle, CR_MethodType);
+
/** {@link ClassDesc} representing the primitive type {@code int}
*/
@Foldable
public static final ClassDesc CR_int = ClassDesc.ofDescriptor("I");
@@ -418,4 +429,36 @@
ClassDesc... paramTypes) {
return MethodHandleDesc.of(STATIC, clazz, name,
MethodTypeDesc.of(returnType, paramTypes).insertParameterTypes(0,
CONDY_BOOTSTRAP_ARGS));
}
+
+ /**
+ * Return a {@link DynamicConstantDesc} which resolves to an
instance of a
+ * functional interface type, as if through an invocation of
+ * {@link LambdaMetafactory#metafactory(MethodHandles.Lookup,
String, Class, MethodType, MethodHandle, MethodType)}
+ *
+ * @param <T> the type of the functional interface
+ * @param invokedName The name of the method to implement.
+ * @param functionalInterface The functional interface type.
+ * @param samMethodType Signature and return type of method to be
implemented
+ * by the function object.
+ * @param implMethod A direct method handle describing the
implementation
+ * method which should be called (with suitable
adaptation
+ * of argument types, return types, and with
captured
+ * arguments prepended to the invocation
arguments) at
+ * invocation time.
+ * @param instantiatedMethodType The signature and return type that
should
+ * be enforced dynamically at
invocation time.
+ * This may be the same as {@code
samMethodType},
+ * or may be a specialization of it.
+ * @return a descriptor that resolves to an instance of the
functional interface type.
+ */
+ @Foldable
+ public static <T> DynamicConstantDesc<T>
ofLambdaCondyBootstrap(String invokedName, ClassDesc
functionalInterface,
+
ConstantDesc<MethodType> samMethodType,
+
ConstantDesc<MethodHandle> implMethod,
+
ConstantDesc<MethodType> instantiatedMethodType) {
+ // Requiring ConstantDesc<...>s here instead of more specific
types since arguments might be
+ // computed at link-time.
+ return DynamicConstantDesc.<T>of(BSM_METAFACTORY_CONDY,
invokedName, functionalInterface)
+ .withArgs(samMethodType, implMethod,
instantiatedMethodType);
+ }
}
```
(Note that some of the javadoc for that last helper method was taken
from the javadoc in `LambdaMetafactory`. I don't know if that matters)
Since I don't have access to `@Foldable` in user code, I found it
impossible to use my own bag-of-constants class, custom descriptor
types or other helper methods, since they would inhibit folding. I think
it's a good move making it an internal annotation though, some weird
problems can occur when using it (e.g. because you're depending on the
compiler's class path), but that also means that currently any helper
method, global constant or descriptor type that someone might need has
to be a part of the standard API.
Best regards,
Jorn Vernee
Vicente Romero schreef op 2018-08-14 02:23:
> On 08/10/2018 04:25 PM, Brian Goetz wrote:
>> Here's my version, similar but with a different error handling
>> strategy:
>>
>> @Override public Optional<?extends ConstantDesc<ConstantDesc<T>>>
>> describeConstable() {
>> ConstantDesc<?>[] args =new ConstantDesc<?>[bootstrapArgs.length
>> +5];
>> args[0] =bootstrapMethod.owner().descriptorString();
>> args[1] =bootstrapMethod.methodName();
>> args[2] =bootstrapMethod.methodType().descriptorString();
>> args[3] =constantName;
>> args[4] =constantType.descriptorString();
>> for (int i=0; i<bootstrapArgs.length; i++)
>> args[i+5] = (ConstantDesc<?>)
>> ((Constable)bootstrapArgs[i]).describeConstable().orElseThrow();
>> return Optional.of(DynamicConstantDesc.of(BSM_DYNAMICCONSTANTDESC,
>> ConstantDescs.DEFAULT_NAME,
>> CR_DynamicConstantDesc, args));
>> }
>>
>>
>> All that said, I'm not sure that we land in the right place here; it
>> is probably more work to LDC this DCD from the constant pool than to
>> construct a fresh one. Its possible we want some more control knobs
>> in Constable to feed back hints into the compiler as to whether or not
>> constantizing is likely to be profitable, or perhaps we need to
>> revisit the constantBoostrap() protocol for DCD.
>>
>> So, excellent bug report -- four issues in one!
>
> yep great bug report and analysis too! getting into this after a time
> out
>
>>
>> - Bug -- compiler incorrectly optimizes away static initializer
>> - Bug -- DynamicConstantDesc.describeConstable() needs to quote its
>> static args
>> - API sharp edge -- varargs version of DCD.of(...)
>> - Possible API insufficiency -- constant-izing DCD may be an
>> anti-optimization
>
> I have pushed fixes to the condy-folding branch for the bugs listed
> above
>
> Thanks,
> Vicente
More information about the amber-dev
mailing list