On Sep 18, 2013, at 2:11 PM, Christian Thalinger
<[email protected]> 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 <[email protected]> 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.
>
_______________________________________________
mlvm-dev mailing list
[email protected]
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
