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

[Vladimir Ivanov]

On Tue, 27 Feb 2024 19:19:17 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>>> 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?
>> 
>> I also would like to learn the downsides. 
>> 
>> You mean `method_data()->is_mature()` was introduced 20 years ago? I didn't capture this. git blame can only trace back to '8153779ad32d1e8ddd37ced826c76c7aafc61894', or "Initial load"  in 2007.
>> 
>> I read the bug description of 'JDK-4292742'. It's something about inconsistent stacktraces between -client and -server. I can't find the patch. I guess C2 uses 'is_mature()' to skip NPE, RCE, DIV0, and casts? Are those function calls? if yes, I think baz() is in the same bucket of them. The only difference is that we want to inline baz() whereas John wants to avoid NPE, RCE etc.
>> 
>>> 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.
>> 
>> I agree. I wonder if we have better approach on this. JRuby has to translate all operations of a Fixnum object.  Some of them are corner cases, but those a handful of rare methods prevent escape analysis from marking 'receiver' NonEscape. 
>>  
>> I don't think '_count field of ciCallProfile = -1' is correct for an immature method. it forces c2 to outline a call. C2 should make judgement based on the information HotSpot has collected. The real frequency is > than `MinInlineFrequencyRatio`. 
>> 
>> One idea I came up is that we add the immature method 'bar' to the dependency list of method foo(). Once c1 collects enough information, we recompile method foo() and see if we get better revision. I gave up because it's complex. 
>> 
>> May I know more about 'excessive recompilation'?  what's your definition of it? 
>> why does using immature methoddata lead to excessive recompilation?
>
>> You mean method_data()->is_mature() was introduced 20 years ago? I didn't capture this. git blame can only trace back to '8153779ad32d1e8ddd37ced826c76c7aafc61894', or "Initial load" in 2007.
> 
> It predates OpenJDK and unfortunately the repository doesn't have any history left from that period. 
> 
>> May I know more about 'excessive recompilation'? what's your definition of it?
> why does using immature methoddata lead to excessive recompilation?
> 
> C2 heavily relies on profiling when performing speculative optimizations. The assumption is the profile is accurate enough to make all the speculations a net win. Optimizing a large enough application consists of numerous individual guesses and some of the predictions routinely fail at runtime causing expensive to recover situations (e.g., "just" an uncommon trap or a signal caused by an implicit exception followed by full reprofile-recompile cycle). If profile data is of a poor quality, it can mislead the compiler to make suboptimal choices and worsen the situation.  
> 
> So, C2 heuristics are tuned to let the profile mature assuming the data becomes more representative over time. (Relevant comment in [methodData.hpp](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/methodData.hpp#L41).)
> 
> Based on the synopsis, the motivation for the fix is to help C2 EA pass. Indeed, there are many situations when a full-blown call in generated code severely affects the quality of optimizations. On the other hand, it a method is inlined prematurely, it may cause a net performance loss as well. For example, when compiled code is invalidated shortly after the compilation, the resources spent on the compilation are wasted  and it slows down application warmup.
> 
> There were some discussions in the past about EA-aware inlining heuristics. Unfortunately, not much progress has been done on that front yet. Partly, because it's notoriously hard and risky to incrementally improve inlining heuristics: they have pervasive effects, so even trivial changes easily cause numerous improvements and regressions across a wide set of applications.
> 
> Speaking of your particular case, as an idea: maybe a static analysis of the callee method may give enough confidence that profile quality doesn't matter there, so it's safe to inline when there are clear benefits for EA? C2 EA already perform some amount of analysis to detect when objects arg-escape (see  [BCEscapeAnalyzer](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/bcEscap...

> I don't think '_count field of ciCallProfile = -1' is correct for an immature method. it forces c2 to outline a call. C2 should make judgement based on the information HotSpot has collected. The real frequency is > than MinInlineFrequencyRatio.

Indeed, that does look excessive. As another idea: utilize frequencies, but not type profiles at call sites with immature profiles. But then the next question is how representative the profile data at callee side then...

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

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