[10] RFR 8186469 MethodHandle.invokeWithArguments jumbo-arity

[Paul Sandoz]

> On 22 Aug 2017, at 02:58, stanislav lukyanov <stanislav.lukyanov at oracle.com> wrote:
> 
> Hi Paul,
> 
> // a non-Reviewer here
> 

Your review is still very welcome and valuable.


> Some Javadoc nits:
> 
> - package-info.Java
> > optionally, between 1 and 251 additional static arguments taken from the constant pool
> Looks like "between 1 and 251" part can be removed.

I missed that, updated to:

* <li>optionally, any number of additional static arguments taken from the constant pool </li>


> It could also say "251 for non-varargs, unlimited for varargs" but it is already discussed in the
> "types of bootstrap methods" section, so can be omitted here.
> 

Right, no need to say more here.


> > In all cases, bootstrap method invocation is as if by
> > {@link java.lang.invoke.MethodHandle#invokeWithArguments MethodHandle.invokeWithArguments},
> > (This is also equivalent to
> > {@linkplain java.lang.invoke.MethodHandle#invoke generic invocation}
> > if the number of arguments is small enough.)
> Now in two places both invoke and invokeWithArguments are mentioned as equivalent "if the number of arguments is small enough",
> and some parts only talk about invoke.
> Can all these references be changed to invokeWithArguments, for simplicity?

Ok, i can replace the second place:

* The bootstrap method will be invoked as if by either {@code MethodHandle.invoke}
* (if there are not too many arguments),
* or {@code invokeWithArguments}.  (There is no way to tell the difference.)

with 

* This bootstrap method will be invoked as if by {@code MethodHandle.invokeWithArguments}.

In the first place i think it is informative to state the relation with invoke.


> I guess the invokedynamic
> specification in JVMS will also only use invokeWithArguments for BSM as well.
> 

Yes, you can see a draft here:

  http://cr.openjdk.java.net/~jrose/jvm/condy-jvms-2017-0620.html


> > For an {@code invokedynamic} instruction (...)
> The context is all about invokedynamic, so this is probably not needed.
> 

I think this is still relevant since we are switching contexts from talking about bootstrap method invocation to the requirements of the invokedynamic processing the value returned by the bootstrap method.

  
> - MethodType.java
> void as a "sentinel type" seems a bit clunky, even though it seems to be safe when used
> in `lastParameterType().getComponentType()` and I can't think of many use cases besides that...
> If the `lastParameterType().getComponentType()` is the only use case, does the method really needs
> to be public?

I think it is genuinely useful, a good sign is that the specification is better for it being there IMO.


> If there are more use cases, is void always safe and not confusing?
> Would Optional be better?
> 

void is safe, it seems reasonable as a sentinel since it flows better than using Optional in this regard.


> Also,
> > @since 9
> @since 10 ? :)

Oops, 9 on the brain… updated.


> 
> - MethodHandle.java
> > <li>Collect the {@code N} elements of the array as a logical
> >     argument list, each argument statically typed as an {@code Object}. </li>
> > <li>Determine, as {@code M}, the parameter count of the type of this
> >     method handle. </li>
> > <li>Determine the general type {@code TN} of {@code N} arguments as
> >     as {@code TN=MethodType.genericMethodType(N)}.</li>
> > <li>If {@code N} is greater than {@code M}, perform the following
> >     checks and actions to shorten the logical argument list: <ul>
> >     <li>Check that this method handle has variable arity with a
> >         {@linkplain MethodType#lastParameterType trailing parameter}
> >         of some array type {@code A[]}.  If not, fail with a
> >         {@code WrongMethodTypeException}. </li>
> >     <li>Collect the trailing elements (there are {@code N-M+1} of them)
> >         from the logical argument list into a single array of
> >         type {@code A[]}, using {@code asType} conversions to
> >         convert each trailing argument to type {@code A}. </li>
> >     <li>If any of these conversions proves impossible, fail with a
> >         {@code WrongMethodTypeException}. </li>
> >     <li>Replace the logical arguments gathered into the array of
> >         type {@code A[]} with the array itself, thus shortening
> >         the argument list to length {@code M}. This final argument
> >         retains the static type {@code A[]}.</li>
> >     <li>Adjust the type {@code TN} by changing the {@code N}th
> >         parameter type from {@code Object} to {@code A[]}.
> >     </ul>
> First, implied `genericMethodType(N)` still fails with N > 255. It could safely be `genericMethodType(M)` though.
> 

Well spotted! I tweaked it to:

* <li>Determine the general type {@code TN} of {@code N} arguments or
*     {@code M} arguments, if smaller than {@code N}, as
*     {@code TN=MethodType.genericMethodType(Math.min(N, M))}.</li>


> Second, it's unfortunate that varargs collection has to be re-implemented in the spec
> for `invokeWithArguments` instead of reusing what `asVarargsCollector` has.

I think this was always the case, but it has become more apparent with what is more clarification than arguably necessary to overcome the argument limitation. There is of course some notable differences between invokeWithArguments and invoke/invokeExact.


> Amusingly, the current spec is almost good enough for unlimited invokeWithArguments except for
> the IAE from `genericMethodType(N)` when N > 255 (*). So, a hackish addition like
>     "Unlike actual calls, the implied `genericMethodType` and `asType` don't fail
>     when a method type with too many parameters is created."
> would actually be sufficient to allow the new behavior. Maybe that's the way to go?
> 
> (*) there is also a corner case of `asType(genericMethodType(255))` that throws WMTE - see JDK-8177275.
> 

Yes, i noticed you commented about that :-)


> > (...) the excess arguments, starting at the position of the trailing
> > array argument, will be gathered (if possible, as if by {@code asType} conversions)
> It doesn't seem necessary for the spec to be verbose about that "regular" varargs processing can
> be reused for M < N < MAX_SAFE and the special processing is only used for N > MAX_SAFE.
> The observable behavior is the same for any N > M, so introducing MAX_SAFE seems redundant.
> 

Yeah, that’s an optimisation that happens to be in the actual implementation as well. How about i move the code into an api note then we can retain that optimisation, as i think it useful to highlight?

Thanks,
Paul.