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

Vitaly Davidovich vitalyd at gmail.com
Wed Oct 28 16:49:33 UTC 2015

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::isaccessor to detect them. > > But then I realized that Method::isaccessor 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::issimpleaccessor; > 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 >

More information about the hotspot-runtime-dev mailing list