[9] RFR (L): 8072008: Emit direct call instead of linkTo* for recursive indy/MH.invoke* calls

John Rose john.r.rose at oracle.com
Thu Nov 19 22:50:59 UTC 2015

On Nov 19, 2015, at 7:25 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> ...
>> /SharedRuntime::resolve_static_call_C/s/static/opt_virtual/ — for
>> invokeBasic, which is not static
> I haven't seen opt_virtual calls for invokeBasic. I think it can be either static or opt_virtual depending on where
> mh.lform.vmentry.vmtarget points to. But right now compiled LFs contain only static method, so it is always compiled into a static call:
> method, so it is e.g.:
>  0x0000000112721797: callq  0x000000011263ba80  ; ImmutableOopMap{}
>                                                ;*invokevirtual
> invokeBasic {reexecute=0 rethrow=0 return_oop=1}
>                                                ; -
> java.lang.invoke.InvokeTest::invokeBasic at 3 (line 138)
>                                                ;   {static_call
> java.lang.invoke.LambdaForm$reinvoker/559450121::dontInline}

(My mistake; I was thinking about the MH::invoke call, not the call it reaches.)

>> Too bad linkToFoo methods don't profile.  Even static methods should
>> profile the first argument (if a reference).  File a bug for this,
>> and point out the place in for_method_handle_inline which suffers?
>> It would help indy also.  I think Roland did most of the work
>> already.
> Are you talking about JDK-8015417: "profile pollution after call through
> invokestatic to shared code" [1]?

Yes, and JDK-6919064.

> What kind of benefits do you expect for MH.linkTo* case?

Now that you ask, "none", for the reasons you suggest.
The linkers are (currently) never called from bytecode with an
interesting profile; the customization mechanism which you
created uses MH constant propagation, so any interesting
profile would have to be derived from the customized MH.

> Not sure profiling would help MH.linkTo* methods, since they are used
> only in compiled LambdaForms for DirectMethodHandles and hence heavily
> shared.

Hmm, yes.  Perhaps there is some value in profiling one or two non-'this'
parameters of the MH invokers.  And of course with one or two indy
argument (JDK-6919064).  Certainly indy calls are rooted in bytecode
with interesting profiles.  It is also possible that some invokeBasic calls
are rooted in customized MH forms with interesting profiles.

> Profile pollution is not the only reason inlining can fail. Inlining
> policy decisions (e.g. recursive call pruning) also affects that.
> Actually, recursive call case was initially reported by Remi.


>> The change near "seemingly deficient out-count" seems like a
>> stand-alone bug fix.  How did you find it?
> The tests I wrote crashed. It was not a problem before, since call site
> argument count matched between bytecode and a method in call generator.
> But as a side effect of replacing MH.linkTo* with a direct/virtual call
> appendix argument is pruned. So, they don't match anymore and I have to
> switch to using bytecode information.

Good.  Squash that bug.

>> Suggest adding asserts somewhere central (doCall?) to check the "it
>> is an error" statements (and maybe move those statements next to the
>> asserts, instead of in the header file).
> Done (see doCall.cpp:607).

I like it.  See below for more comments on the assert logic.

>> The _invokeBasic intrinsic corresponds to Bytecode::_invokehandle,
>> not Bytecode::_invokestatic.  The sharedRuntime code associates it
>> with invokestatic.  This seems murky to me.
> Fixed.
> It's invokehandle on bytecode level, but after the compiler inlines
> through the call, it becomes a direct call in compiled code.
> Feeding Bytecode::_invokehandle into LinkResolver doesn't work, since it
> expects resolved klass to be MethodHandle. But it is not the case
> anymore, since the compiler already eliminated the indirection.
> Do you see a better solution than patching bc before performing method resolution?

The most important thing the bc does is carry the distinction between
linkToSpecial and linkTo{Virtual,Interface}.  The bc also tells whether
there is a receiver in the mix, which gates a null check.

Maybe these two aspects of bc need to be clarified, by factoring them
into a bc and a flag (either "has_receiver" or "null_check_receiver").

Would it work to consolidate the logic which derives invoke_bc from bc
to the place where bc is first computed?  The two derivations of bc
and later invoke_bc seem awkward to me.

In fact, now I wonder how an inlined linkToSpecial will perform a receiver
null check correctly, in the present logic.


>> It would be much better if there were one call to retrieve_receiver
>> instead of two.  The duplication of logic is confusing.  Strongly
>> suggest factoring the existing logic into a common subroutine call.
> I've refactored SharedRuntime::find_callee_info_helper to avoid repeated
> work and cover attached resolved method case more uniformly.

Thank you.

>> Also, the near-copy resolve_attached_call_info of
>> LinkResolver::resolve_invoke (in a different file!) is frustratingly
>> confusing.  ("Now, where have I seen that switch statement before?")
>> I suggest factoring them together somehow, or at least putting the
>> twin copies side by side in linkResolver.cpp.
> Moved resolve_attached_call_info to LinkResolver::resolve_invoke overload.

Good.  But this signature seems odd to me, since LinkResolver results
are leading parameters.

+   static void resolve_invoke(methodHandle info, Bytecodes::Code bc,
+                              Handle& receiver, CallInfo& callinfo, TRAPS);

Also, we use "const methodHandle&" more these days.

To maximize correspondence with the neighboring function, maybe this?

+   static void resolve_invoke(CallInfo& result, Handle recv,
                             const methodHandle& attached_method,
                             Bytecodes::Code byte, TRAPS);

>> Basically, the logic says:  a. get receiver r1, b. resolve method m1,
>> c. if it is a redirectable intrinsic, then: d. get receiver r2, e.
>> get attached method m2, f. resolve method m2.  This is too
>> complicated for what is really intended, which is (get attached
>> method || get symbolic method) && get receiver && resolve.
>> The whole thing about the dynamic receiver doesn't really belong in
>> the static resolution mix, except that we are supporting monomorphic
>> inline caches along this path.  I'd rather get the method completely
>> determined, and then (if needed) do the receiver trick.
> I tried to achieve that with SharedRuntime::find_callee_info_helper
> refactoring. Do you like how it is shaped now?

Much better; I like the parallel tracks of attached method vs. CP method.
It gives the two items consistent roles.

But perhaps the bc is too overloaded; see above.

>> Where else in the system is there code like
>> extract_attached_call_info (which loops over reloc-info looking for
>> something)?  Most uses of RelocIterator are in codeBlob, nmethod, or
>> relocInfo, and the only ones in sharedRuntime are all about call site
>> patching, not (initial) linking.  Suggest putting it in nmethod or
>> relocInfo, so it is easy to find.
> Moved iteration logic into nmethod::attached_method().

Much better.

>> Again, this is a good change.  Let's treat symbolic method
>> references and attached methods as symmetrically as possible.  The
>> result will clarify the JVM's core logic, IMO, since currently
>> bytecode unpacking is mixed confusingly into the immediately
>> following phase of method resolution.
> I've spent some time thinking about how to treat attached methods more uniformly and avoid specializing them for MH linkers case. But it requires additional information along with attached method - invocation mode (to be able to resolve a method w.r.t. receiver). It would make SharedRuntime::find_callee_info_helper completely MH agnostic. But does it worth it?

We need invocation mode and (independently) null check.  See if the logic clarifies if you separate those aspects.

> Best regards,
> Vladimir Ivanov
> [1] https://bugs.openjdk.java.net/browse/JDK-8015417

On Nov 19, 2015, at 8:04 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>> Maybe a special check in CallGenerator::generate() to catch failed
>> inlines through MH.linkTo/invokeBasic?
> ... and it looks pretty decent:
>  http://cr.openjdk.java.net/~vlivanov/8072008/webrev.02_03 <http://cr.openjdk.java.net/~vlivanov/8072008/webrev.02_03>

Yes, I agree that is clearer.

But I think you need to adjust is_call_consistent_with_jvms some more.
The call to is_inlined_mh_linker appears to be in a place where it
can never return true.

Also, please consider walking the parallel argument lists for the MH calls
to check for oop/32/64 mismatches.  You can do it easily with a pair of
SignatureStream iterators.

— John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20151119/f63bcdcc/attachment-0001.html>

More information about the hotspot-compiler-dev mailing list