RFR (S) 8140650: (preliminary) Method::is_accessor should cover getters and setters for all types

[Aleksey Shipilev]

Well, if I read the current inlining policy right, accessors are exempt
from MaxInlineLevel checks, profile hotness checks (accessors are
assumed hot), etc.

Since inlining accessors does not bloat the IR and/or generated code
(which conservative inlining policy is protecting against), it seems
silly to ignore these easy inline targets. This is why, I think,
inlining policy treats accessors differently.

The actual issue is not about treating the accessors in compiler, but
rather about runtime disregarding many methods which are, in fact,
trivial accessors.

Thanks,
-Aleksey

On 10/28/2015 07:16 PM, Vitaly Davidovich wrote:
> Under what practical conditions would they fail to inline otherwise,
> given how tiny they are? MaxInlineSize already disregards frequency.
> 
> sent from my phone
> 
> On Oct 28, 2015 11:32 AM, "Aleksey Shipilev"
> <aleksey.shipilev at oracle.com <mailto:aleksey.shipilev at oracle.com>> wrote:
> 
>     Hi,
> 
>     I have been reading the compiler code recently to check if
>     setters/getters are actually treated specially in inline policy. They
>     do, and inliner relies on Method::is_accessor to detect them.
> 
>     But then I realized that Method::is_accessor implementation only accepts
>     the specific shapes of getters, and completely ignores setters (contrary
>     to what is spelled in the "doc" comment!):
>       https://bugs.openjdk.java.net/browse/JDK-8140650
> 
>     This makes compilers to ignore many trivial methods that we might
>     otherwise inline when all other inline hints have failed. With that in
>     mind, I did a proof-of-concept change, which passes JPRT and a new
>     compiler-specific regression test:
>       http://cr.openjdk.java.net/~shade/8140650/webrev.00/
>     <http://cr.openjdk.java.net/%7Eshade/8140650/webrev.00/>
> 
>     I'll run more testing, after we figure the fate for interpreter.cpp
>     change. It is prompted by Zero's fast accessor implementation that only
>     accepts the specific getter shape. Now, we can go three routes:
>      a) Ignore the issue, and keep Method::is_simple_accessor;
>      b) Fix Zero's fast accessor to accept all the shapes.
>      c) Remove fast accessors from Zero (I see UseFastAccessorMethods is
>     marked as obsolete), and thus probably remove the notion of "accessor"
>     from interpreter completely (?);
> 
>     Current patch does (a), and I'm leaning to keep it that way, letting
>     Zero to handle more in future. If we care more about Zero, we might go
>     for (b) -- although it seems to deserve a separate follow-up RFE. And if
>     we don't, we can go for (c).
> 
>     Thoughts?
> 
>     Thanks,
>     -Aleksey
>