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

[Vitaly Davidovich]

There's also code in the inline policy which skips a bunch of checks if
code_size <= MaxTrivialSize, which defaults to 6 -- this is right below the
is_accessor check, and also talks about trivial methods and accessors.  All
in all, it seems like these trivial accessors ought to get inlined anyway,
and I'm having a hard time picturing a case where they wouldn't.  I'm not
entirely sure why there are so many seemingly duplicate concepts checked in
the inlining policy (e.g. is_accessor vs MaxTrivialSize -- does someone
really turn MaxTrivialSize down below 6??).

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


You mean in interpreter? Where else does this play a role?


On Wed, Oct 28, 2015 at 12:32 PM, Aleksey Shipilev <
aleksey.shipilev at oracle.com> wrote:

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