[XXS] C1 misses to dump a reason when it inlines successfully

[Doerr, Martin]

Hi,

looks good to me. Thanks for cleaning it up.

Best regards,
Martin


From: Liu Xin <navy.xliu at gmail.com>
Sent: Freitag, 29. November 2019 08:17
To: Doerr, Martin <martin.doerr at sap.com>
Cc: Tobias Hartmann <tobias.hartmann at oracle.com>; hotspot-compiler-dev at openjdk.java.net
Subject: Re: [XXS] C1 misses to dump a reason when it inlines successfully

Hi, Tobias & Martin,

The original message even didn't say inline succeed or not. When I see those, I have to dig into the source code. I spent even more time because I didn't realize that those messages are from C1. That's my standpoint and motivation as a new hotspot developer.
What C2 inliner emits "inline (hot)"  is not very informative neither, but at least developers know that this callee is inlined. I gave up "by the rules of C1". I think "inline" is fine. developers can also tell difference from C2's output.

I agree with you about the default parameter.  Failed attempts should always have a reason. Developers can inspect them and make adjustment.

Here is a new revision. Could you take a look?
https://cr.openjdk.java.net/~xliu/8234541/01/webrev/

sample output is like this.
                              @ 1   java.lang.String::isLatin1 (19 bytes)   inline
                              @ 12   java.lang.StringLatin1::charAt (28 bytes)   inline
                                @ 15  java/lang/StringIndexOutOfBoundsException::<init> (not loaded)   not inlineable
                              @ 21  java/lang/StringUTF16::charAt (not loaded)   not inlineable
                              @ 15  java/lang/StringIndexOutOfBoundsException::<init> (not loaded)   not inlineable
                              @ 3   java.util.stream.ReduceOps::makeInt (18 bytes)   inline
                                @ 1   java.util.Objects::requireNonNull (14 bytes)   inline
                                  @ 8   java.lang.NullPointerException::<init> (5 bytes)   don't inline Throwable constructors
                                @ 14   java.util.stream.ReduceOps$6::<init> (16 bytes)   inline
                                  @ 12   java.util.stream.ReduceOps$ReduceOp::<init> (10 bytes)   inline
                                    @ 1   java.lang.Object::<init> (1 bytes)   inline
                              @ 6   java.util.stream.AbstractPipeline::evaluate (94 bytes)   callee is too large
thanks,
--lx



On Thu, Nov 28, 2019 at 6:53 AM Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi,

I agree with Tobias. " by the rules of C1" doesn't add any useful information IMHO.
But I'd be fine with just adding "inline" to avoid the confusion.

> Anyway, with your change, all callers now pass a msg argument and
> therefore the null handling logic
> should be removed (replaced by an assert). Also, the default value for the
> argument can be removed.
+1

Best regards,
Martin


> -----Original Message-----
> From: hotspot-compiler-dev <hotspot-compiler-dev-
> bounces at openjdk.java.net<mailto:bounces at openjdk.java.net>> On Behalf Of Tobias Hartmann
> Sent: Donnerstag, 28. November 2019 15:04
> To: Liu Xin <navy.xliu at gmail.com<mailto:navy.xliu at gmail.com>>; hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: [XXS] C1 misses to dump a reason when it inlines successfully
>
> Hi,
>
> I'm still not convinced that this message adds any useful information but let's
> see what other
> reviewers think.
>
> Anyway, with your change, all callers now pass a msg argument and
> therefore the null handling logic
> should be removed (replaced by an assert). Also, the default value for the
> argument can be removed.
>
> Best regards,
> Tobias
>
> On 22.11.19 19:09, Liu Xin wrote:
> > hi, Reviewers,
> >
> > Could you review this extremely small change?
> > Bugs: https://bugs.openjdk.java.net/browse/JDK-8234541
> > Webrev: https://cr.openjdk.java.net/~xliu/8234541/00/webrev/
> >
> > When I analyzed PrintInlining, I was confused by the inline message
> without
> > any detail. It's not easy for developer to tell if this method is inlined
> > or not. This patch add a comment "inline by the rules of C1".
> >
> > I would like to add an explicit reason, but there's no decisive reason in
> > GraphBuilder::try_inline_full. It just passes all restrict rules. Any other
> > suggestion would be appreciated.
> >
> > Thanks,
> > --lx
> >