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

[Aleksey Shipilev]

On 10/28/2015 07:49 PM, Vitaly Davidovich wrote:
> 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. 

If you take a closer look, then you'll notice that a large number of
checks is done for both trivial and non-trivial methods, after the ">
MaxTrivialSize" block ends. AFAIR, this is because method size *does
not* tell e.g. how many nodes it will bring: there might be a method
call, or special code that is intrinsified, or... etc, etc, etc.
Accessors are much safer in this regard.

You also miss the entire story about the interaction with profile
hotness, see e.g. InlineTree::should_not_inline and checks via
Method::was_executed_more_than. Cold "trivial" methods would not be
inlined, but accessors would. In vast majority of cases the profile
itself would tell to inline the accessor method, even if accessor check
fails. But the profile is not a completely reliable instrument for
inlining policy, as even the cold methods may break optimizations
downstream.

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

I am saying that the goal for this change is *not* about either
discussing or changing the inlining policy decisions within the
compiler. Accessors are treated differently for a reason. This issue is
about matching all actual accessors, not some small subset of them.

Let's not blow up the contained tech debt improvement into a full-scale
overhaul of the inlining heuristics.

Thanks,
-Aleksey