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

[Vitaly Davidovich]

>
> 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 (?);


Another option to consider is #ifdef is_accessor for Zero differently from
the rest so you don't need two different methods.


On Wed, Oct 28, 2015 at 11:31 AM, Aleksey Shipilev <
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/
>
> 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
>
>