8267807: C2: Downcast receiver to target holder during inlining #4212
Conversation
|
/label add hotspot-compiler |
|
👋 Welcome back vlivanov! A progress list of the required criteria for merging this PR into |
|
@iwanowww |
| bool has_receiver = !cha_monomorphic_target->is_static(); | ||
| bool is_interface_holder = cha_monomorphic_target->holder()->is_interface(); | ||
| if (has_receiver && !is_interface_holder) { | ||
| if (!cha_monomorphic_target->holder()->is_subtype_of(receiver_type->klass())) { |
There was a problem hiding this comment.
Given we can't trust interface types, shouldn't we test for receiver_type not an interface here?
There was a problem hiding this comment.
Though it doesn't happen in practice now, we can trust interface types on the receiver object because virtual/interface method invocation implies proper type check of the receiver.
There was a problem hiding this comment.
I think you need a CheckCastPP as the comment (written by me long ago IIRC) suggests.
I’d like to believe you can trust the type of a CHA receiver, but I don’t. What the IR says about interface types is (last time I looked) just as untrustworthy as what the verifier thinks about them.
(We should fix this decisively someday. Or did we do this cleanup already and missed it? We have had dozens of bugs
in early C2 because of untrustworthy interface types.)
Suppose CHA (of default methods, yay!) says that since x is interface Foo, then default Foo::m is the only possible target
of x.m. Before we execute Foo::m we still need a type check of x, because it could be any Object whatsoever.
I suggest adding the CheckCastPP in, in all modes, with fallback to de-opt not VM crash. The IR types are saying there is not a dominating check.
There was a problem hiding this comment.
I closely looked at the relevant logic and couldn't find any possibility to bypass receiver subtype check.
There are 2 call site shapes possible:
invokevirtual C.m RECV => J.m DEFAULT(J.m- resolved and selected method)
Verifier guarantees that RECV <: C (REFC) and method resolution ensures that C <: J (REFC <: DECC).
invokeinterface I.m RECV => J.m DEFAULT(J.m- resolved and selected method)
I (REFC) is required to be an interface (structural constraint of invokeinterface instruction).
Then, here's the relevant code:
ciInstanceKlass* actual_receiver = resolved_iklass;
...
ciInstanceKlass *ikl = receiver_type->klass()->as_instance_klass();
if (ikl->is_loaded() && ikl->is_initialized() && !ikl->is_interface() &&
(ikl == actual_receiver || ikl->is_subtype_of(actual_receiver))) {
// ikl is a same or better type than the original actual_receiver,
// e.g. static receiver from bytecodes.
actual_receiver = ikl;
// Is the actual_receiver exact?
actual_receiver_is_exact = receiver_type->klass_is_exact();
}
ciInstanceKlass* calling_klass = caller->holder();
ciMethod* cha_monomorphic_target = resolved_method->find_monomorphic_target(calling_klass, resolved_iklass, actual_receiver, check_access);
...
ciMethod* ciMethod::find_monomorphic_target(...) {
...
if (actual_recv->is_interface()) {
// %%% We cannot trust interface types, yet. See bug 6312651.
return NULL;
}
receiver_type can represent either an interface (1a) or a class (1b ikl).
1a. But if ikl->is_interface(), then actual_receiver stays as an interface (I, REFC) and find_monomorphic_target() returns NULL.
1b. If ikl is a class, then ikl != actual_receiver and depending on whether ikl->is_subtype_of(actual_receiver)), actual_receiver is either REFC (interface) or receiver_type.
find_monomorphic_target() can succeed only in the latter case (actual_receiver is a class and not an interface), but previous static subtype check guarantees that the receiver type (a class) implements the interface.
Q.E.D
There was a problem hiding this comment.
So, CHA is still kept conservative when interface types on receiver are encountered.
Currently, only single implementor case is optimised, but there are proper receiver subtype checks in place.
It should be quite straightforward to relax aforementioned constraints on receiver types (only classes, no interfaces), but then we would require additional receiver subtype check accompanying Compile::optimize_virtual_call() calls.
There was a problem hiding this comment.
Anyway, as a defense in depth measure, we could still insert receiver subtype check when CHA returns a default method and hope that the check will go away once the type system better tracks interface types. Though the subtype check against an interface can be quite expensive, I don't expect any performance regressions considering CHA hasn't optimized default methods before.
There was a problem hiding this comment.
Evidently, it's not the whole story :-) Otherwise, we wouldn't have needed downcasts at all.
It becomes more interesting when CHA discovers unique concrete method under REFC:
3a. invokevirtual C.m RECV => J.m ABSTRACT, unique = K.m DEFAULT, K <: J (I.m - resolved method, REFC = C)
3b. invokeinterface I.m RECV => J.m ABSTRACT, unique = K.m DEFAULT, K <: J (I.m - resolved method, REFC = I)
(Moreover, 3b is equivalent to 3a since actual_receiver has to be a class and
not an interface. Otherwise, CHA fails fast. There's additional case - unique
implementor - but it strength-reduces invokeinterface to invokevirtual.)
It is guaranteed that RECV <: C <: J and RECV <: I < J, but there's nothing except the CHA itself that
proves RECV <: J always holds.
In 3a and 3b case, CHA iterates all the subtypes under statically known receiver type
(actual_receiver, subtype of I/C). Assuming the analysis is correct, it proves
that RECV <: J.
There was a problem hiding this comment.
As a summary: if we feel safer when there's a dynamic receiver subtype check always present, it could be added.
We would pay with performance (the check can become quite costly in some corner cases), but it shouldn't introduce performance regressions (compared to previous releases): CHA didn't return default methods before and interface call involves implicit receiver type check.
(Need to be cautious not to replace a type profile-guided monomorphic case with a statically determined target interface method + interface subtype check. So, additional complexity in implementation as well.)
|
@iwanowww This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 86 new commits pushed to the
|
Existing tests (and new CHA tests under review) trigger the assert in Once IR testing framework is integrated, it may be worth the effort to prepare a stand-alone simple test case. |
|
Okay, thanks for the details! |
|
Thanks for the reviews, Roland and Tobias. /integrate |
openjdk
bot
added
integrated
|
@iwanowww Since your change was applied there have been 92 commits pushed to the
|
Virtual method calls involve an implicit subtype check against callee holder.
But if receiver type is too broad, it has to be narrowed before parsing the callee method.
Otherwise, it may cause problems during parsing and currently it simply blocks inlining.
Proposed fix implements the narrowing step and re-enables inlining.
Testing:
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4212/head:pull/4212$ git checkout pull/4212Update a local copy of the PR:
$ git checkout pull/4212$ git pull https://git.openjdk.java.net/jdk pull/4212/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4212View PR using the GUI difftool:
$ git pr show -t 4212Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4212.diff