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

[John Rose]

On Nov 5, 2015, at 7:08 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> 
> Any (R)eviews, please?

I really like what this *does*, but I have a bunch of nit-picky remarks on how it is formulated.
Since this is at the heart of the JVM's linkage logic, it's important to keep it clean.

 I'm slightly uncomfortable with making CallGenerator into a state-ful type.  Maybe:

-+   cg->set_override_symbolic_info(true);
++   cg = cg->with_override_symbolic_info(true);

To do this cleanly requires a virtual (re-constructor) with lots of defs.

Perhaps it can be done dirtily also, as long as there is a note saying "if these objects are ever cached, a fresh copy must be created here", inside the dirty implementation of with_override_symbolic_info.

(Or add virtual CallGenerator::fresh_copy, and keep the side-effecting thing?)

John Rose: [11/11/15, 6:06:11 PM]

/SharedRuntime::resolve_static_call_C/s/static/opt_virtual/ — for invokeBasic, which is not static

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.

The change near "seemingly deficient out-count" seems like a stand-alone bug fix.  How did you find it?

Suggest a comment in callGenerator.hpp:

// If override_symbolic_info is true, then the CG::_method value may differ from the symbolic reference at the current bytecode, and the runtime linker (e.g., SharedRuntime::resolve_static_call_C) will be told to consult the CG's method, not the current bytecode's method.  It is an error if the methods have incompatible descriptors, or if the methods differ but override_symbolic_info is false.

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).

The _invokeBasic intrinsic corresponds to Bytecode::_invokehandle, not Bytecode::_invokestatic.  The sharedRuntime code associates it with invokestatic.  This seems murky to me.

Suggest renaming compute_bc to MethodHandles::signature_polymorphic_intrinsic_bytecode (next to spi_name).

It's way confusing to have variables "callinfo" and "call_info".  Suggest s/call_info/attached_method/.  Likewise s/extract_attached_call_info/extract_attached_method/.  And s/resolve_attached_call_info/resolve_attached_method/.

s/use it's holder/use its holder/  — Its genitive is "its"; yes it's confusing.

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.

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.

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.

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.

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.

Thanks,
— John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20151111/dde61513/attachment.html>