RFR (L) 8024761: JSR 292 improve performance of generic invocation

[John Rose]

On Sep 18, 2013, at 2:11 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:

> src/share/classes/java/lang/invoke/CallSite.java:
> 
> +                    if (3 + argv.length > MethodType.MAX_MH_ARITY)
> +                    MethodType invocationType = MethodType.genericMethodType(3 + argv.length);
> +                    MethodHandle spreader = invocationType.invokers().spreadInvoker(3);
> 
> Could we use a defined constant for "3"?

Yes.
> 
> src/share/classes/java/lang/invoke/Invokers.java:
> 
> +        if (targetType == targetType.erase() && targetType.parameterCount() < 10)
> 
> The same here for "10".

Fixed; factored to a subroutine.

> Actually, exactInvoker and generalInvoker's code could be factored into one method.
> 
> +    /*non-public*/ MethodHandle basicInvoker() {
> +        //invoker.form.compileToBytecode();

Done.

> Please remove commented lines.
> 
> +    static MemberName exactInvokeLinkerMethod(MethodType mtype, Object[] appendixResult) {
> +    static MemberName genericInvokeLinkerMethod(MethodType mtype, Object[] appendixResult) {
> 
> These two could also be factored into one method.

Done.  I removed those two, and called the one new method from MethodHandleNatives.

> +    // Return an adapter for invokeExact or generic invoke, as a MH or constant pool linker
> +    // mtype : the caller's method type (either basic or full-custom)
> +    // customized : whether to use a trailing appendix argument (to carry the mtype)
> +    // which&0x01 : whether it is a CP adapter ("linker") or MHs.invoker value ("invoker")
> +    // which&0x02 : whether it is for invokeExact or generic invoke
> +    //
> +    // If !customized, caller is responsible for supplying, during adapter execution,
> +    // a copy of the exact mtype.  This is because the adapter might be generalized to
> +    // a basic type.
> +    private static LambdaForm invokeHandleForm(MethodType mtype, boolean customized, int which) {
> 
> Why are you not using Javadoc style for this method comment?  It's still helpful in IDEs.

Fixed.

> src/share/classes/java/lang/invoke/LambdaForm.java:
> 
>     static void traceInterpreter(String event, Object obj, Object... args) {
> +        if (!(TRACE_INTERPRETER && INIT_DONE))  return;

Done.
> 
> Why not use the same pattern:
> 
> +        if (TRACE_INTERPRETER && INIT_DONE)
> 
> as the other places?
> 
> +    static final boolean INIT_DONE = Boolean.TRUE.booleanValue();
> 
> Why are we having this after all?

I removed it; see webrev.  There's a comment which explains the problem and the nicer replacement.

> src/share/classes/java/lang/invoke/MemberName.java:
> 
> +    public MemberName asNormalOriginal() {
> 
> Could you add a comment to this method?  It's not clear to me what "normal" and "original" mean here.

Done.

> +    public MemberName(byte refKind, Class<?> defClass, String name, Object type) {
> +        @SuppressWarnings("LocalVariableHidesMemberVariable")
> +        int kindFlags;
> 
> The SuppressWarnings is in the other constructors because of the name "flags".  You don't need it here.  Maybe rename the others as well and get rid of the annotation.

OK; I removed 2/3 of them.

> src/share/classes/java/lang/invoke/MethodHandleNatives.java:
> 
>     static String refKindName(byte refKind) {
>         assert(refKindIsValid(refKind));
> -        return REFERENCE_KIND_NAME[refKind];
> +        switch (refKind) {
> 
> After this change REFERENCE_KIND_NAME is not used anymore.

Good catch.

> src/share/classes/java/lang/invoke/MethodHandles.java:
> 
> +            member.getName().getClass(); member.getType().getClass();  // NPE
> 
> Please don't!  It would be nice to have at least a different line number in the backtrace.

OK.  I split three such lines.

I would like to switch all these to Objects.requireNonNull, but only after a little performance testing to make sure we don't have surprises.

> src/share/classes/java/lang/invoke/MethodTypeForm.java:
> 
> +            //Lookup.findVirtual(MethodHandle.class, name, type);
> 
> Either remove it or add a comment why it's there.

Commented better now.

While testing, I found a spot in BoundMethodHandle that needed a tweak to avoid a bootstrap problem.  (Too many of those.)

I sharpened the type of 'caller' in CallSite.makeSite.

The webrev is updated:
  http://cr.openjdk.java.net/~jrose/8024761/webrev.01/

Thanks!

— John

> On Sep 12, 2013, at 6:36 PM, John Rose <john.r.rose at oracle.com> wrote:
> 
>> Please review this change for a change to the JSR 292 implementation:
>> 
>> http://cr.openjdk.java.net/~jrose/8024761/webrev.00/
>> 
>> Bug description:  The performance of MethodHandle.invoke is very slow when the call site type differs from the method handle type. When the types differ, the invocation is defined to proceed as if two steps were taken: 
>> 
>> 1. the target method handle is first adjusted to the call site type, by MethodHandles.asType 
>> 
>> 2. the type-adjusted method handle is invoked directly, by MethodHandles.invokeExact 
>> 
>> The existing code (from JDK 7) awkwardly performs the type adjustment on every call. But performing the type analysis and adapter creation on every call is inherently slow. A good fix is to cache the result of step 1 (MethodHandles.asType), since step 2 is already reasonably fast. 
>> 
>> For most applications, a one-element cache on each individual method handle is a reasonable choice. It has the particular advantage of speeding up invocations of non-varargs bootstrap methods. To benefit from this, the bootstrap methods themselves need to be uniquified across multiple class files, so this work will also include a cache to benefit commonly-used bootstrap methods, such as JDK 8's LambdaMetafactory.metafactory. 
>> 
>> Additional caches could be based on the call site, the call site type, the target type, or the target's MH.form. 
>> 
>> Thanks,
>> — John
>> 
>> P.S. Since this is an implementation change oriented toward performance, the review request is to mlvm-dev and hotspot-compiler-dev.
>> Changes which are oriented toward functionality will go to mlvm-dev and core-libs-dev.
>