On Sep 18, 2013, at 6:43 PM, John Rose <john.r.r...@oracle.com> wrote:
> On Sep 18, 2013, at 2:11 PM, Christian Thalinger
> <christian.thalin...@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.
Good comment. Thank you.
>
>> 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/
This looks good.
>
> Thanks!
>
> — John
>
>> On Sep 12, 2013, at 6:36 PM, John Rose <john.r.r...@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.
>>
>
> _______________________________________________
> mlvm-dev mailing list
> mlvm-dev@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
