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

[Severin Gehwolf]

Hi Aleksey,

Thanks for bringing this to our attention!

On Fri, 2015-10-30 at 17:21 +0300, Aleksey Shipilev wrote:
> Hi,
> 
> There is a suggested change that affects Zero: UseFastAccessorMethods is
> used only in Zero after JDK-8003426. We would like to make the
> is_accessor matcher a proper one, which raises some questions below:
> 
> -------- Forwarded Message --------
> Subject: RFR (S) 8140650: (preliminary) Method::is_accessor should cover
> getters and setters for all types
> Date: Wed, 28 Oct 2015 18:31:46 +0300
> From: Aleksey Shipilev <aleksey.shipilev at oracle.com>
> To: hotspot-runtime-dev <hotspot-runtime-dev at openjdk.java.net>
> 
> 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?

It should be fine to go with (a) and also file a follow-up RFE for Zero
in order to implement (b). I can take this task on if you wish. Please
let me know about the RFE bug you file.

Thanks,
Severin