RFR: 8325681: C2 inliner rejects to inline a deeper callee because the methoddata of caller is immature. [v2]

[Vladimir Ivanov]

On Mon, 26 Feb 2024 04:05:11 GMT, Xin Liu <xliu at openjdk.org> wrote:

>> This patch uses the methoddata of a method no matter it is mature or not to initialize `ciCallProfile`. Previously, C2 drops premature methoddata and leaves _count field of ciCallProfile -1. This leads C2 refuses to inline the callsite because its frequency is too low(-1 < MinInlineFrequencyRatio).  
>> 
>> In the given example, we observes that baz was not inlined because of 'low call site frequency'.  This is wrong because its real frequency is 10% > MinInlineFrequencyRatio. 
>> 
>> 
>> 60 13 b 4 UnderProfiledSubprocedure::foo (9 bytes)
>>                               @ 5 UnderProfiledSubprocedure::bar (6 bytes) inline (hot)
>>                                 @ 1 UnderProfiledSubprocedure::baz (19 bytes) failed to inline: low call site frequency
>
> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update the comment.

src/hotspot/share/ci/ciMethod.cpp line 466:

> 464:   ciCallProfile result;
> 465:   // take CounterData regardless of maturity.
> 466:   if (method_data() != nullptr) {

You propose to completely eliminate MDO maturity check for call site profiles. You do get desired inlining happening, but what are the downsides? 

The check was added as part of JDK-4292742 fix and special attention was paid there to avoid excessive trapping behavior. How much the removal worsens the situation?

Personally, I'd be more comfortable with a more nuanced fix. Inlining a method with immature profile does look like a source of excessive recompilations in large applications.

test/hotspot/jtreg/compiler/c2/irTests/TestUnderProfiledSubprocedure.java line 61:

> 59:     Object[] setupCondition() {
> 60:         // return true with ODD% possibility.
> 61:         return new Object[]{Boolean.valueOf(RANDOM.nextInt(100) < ODD)};

Why don't you produce a deterministic sequence with desired distribution instead?
Random input data generation may be a source of intermittent test failures. 
E.g., what are the chances that condition will never be true? 
(It's the first time I see `@Setup` usage, so I have a hard time reasoning about its behavior.)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17957#discussion_r1503367812
PR Review Comment: https://git.openjdk.org/jdk/pull/17957#discussion_r1503367723