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

[Vladimir Ivanov]

Thanks, Tobias.

Updated webrev in-place.

Best regards,
Vladimir Ivanov

On 30/01/2019 01:21, Tobias Hartmann wrote:
> 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