[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.
Yes.
>
>> 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