[13] RFR (M): 6986483: CHA: optimize calls through interfaces

[Tobias Hartmann]

Hi Vladimir,

this looks good to me!

Just spotted some typos in the comments:
- ciInstanceKlass.hpp: "than one implementors" -> "than one implementor"
- doCall.cpp: "may be able bind" -> "may be able to bind"
  "is the it's supposed" -> "is that it's supposed"
- The copyright date in the test is wrong

Best regards,
Tobias


On 29.01.19 23:09, Vladimir Ivanov wrote:
> Thanks, Nils.
> 
> Additional testing revealed one problematic corner case - Object methods on interfaces. The
> transformation is not valid for them, because it eliminates proper receiver subtype check: subtype
> check against Object is a no-op.
> 
> Updated version:
>   http://cr.openjdk.java.net/~vlivanov/6986483/webrev.02/
> 
> The changes in c1_GraphBuilder.cpp and doCall.cpp are trivial (cha_monomorphic_target->holder() !=
> Object_klass()), but I extended the test with more cases.
> 
> Testing: hs-precheckin-comp, tier1-5
> 
> Best regards,
> Vladimir Ivanov
> 
> On 29/01/2019 06:03, Nils Eliasson wrote:
>> Hi Vladimir,
>>
>> A really good improvement, and I really like the test, excellent coverage.
>>
>> Reviewed,
>>
>> // Nils
>>
>>
>> On 2019-01-25 22:27, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~vlivanov/6986483/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-6986483
>>>
>>> Another candidate for revival. At that time it was reviewed, but integration was blocked pending
>>> another bug fix. Now the fix is in.
>>>
>>> Quote from original review request [1]:
>>>
>>> "Proposed change adds CHA support in C2 for interface calls.
>>>
>>> Consider the following hierarchy:
>>>
>>>    interface Intf { m(); }
>>>    class C implements Intf { public m() { ... } }
>>>    class C1 extends C { /* doesn't override m() */ }
>>>    ...
>>>    class Cn extends C { /* doesn't override m() */ }
>>>
>>> Call site: invokeinterface Intf.m() ...
>>>
>>> If Intf were an abstract class, CHA could deduce that Intf::m() can be
>>> replaced with C::m(), but it doesn't work for interfaces. Verifier
>>> doesn't check interface types in bytecode, so CHA can't assume the
>>> receiver implements Intf.
>>>
>>> CHA in C1 handles such call sites for interfaces with a single
>>> implementor. It replaces invokeinterface Intf.m() with invokevirtual
>>> C.m() guarded by a subtype check (instanceof C). C2 doesn't do that and
>>> this request is about adding that. Type profiling doesn't help here (the
>>> call site is usually megamorphic), so C2 can't inline it.
>>>
>>> The proposed implementation is similar to C1, except that the code
>>> deoptimizes when subtype check fails and ICCE is thrown from the
>>> interpreter.
>>>
>>> While working on it, I spotted and fixed a couple of inefficiencies in
>>> C1 implementation:
>>>
>>>    (1) dependency context being used was broader than necessary -
>>> resolved instead of declared interface (hence, possibility of
>>> unnecessary invalidations);
>>>
>>>    (2) didn't work for interfaces w/ any default methods: CHA doesn't
>>> support default methods at the moment, so what matters is whether
>>> Intf::m() is default or not and not whether Intf has *any* concrete methods."
>>>
>>>
>>> Testing: hs-precheckin-comp, hs-tier1, hs-tier2
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1] https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-February/025630.html