review(M): 7057120: Tiered: Allow C1 to inline methods with loops

[Vladimir Kozlov]

Igor Veresov wrote:
> Thanks for the review, Vladimir! Webrev updated. Please find the replies 
> inline.
> 
> On 6/22/11 9:51 AM, Vladimir Kozlov wrote:
>> Can you explain why in counter_overflow_helper() (c1_Runtime1.cpp) you
>> expect the caller (enclosing_method) to be nmethod?
> 
> It's not the caller. The enclosing_method is the actual method to which 
> the nmethod belongs. In other words: enclosing_method is a method who's 
> frame is current on stack and "method" is possibly the inlinee inside 
> the enclosing method that generated the event.

It would be better to say this in the comment for counter_overflow_helper() 
method since the current comment does not help at all. I don't see
anything related to safepoint.

> 
>>
>> Could you rename should_not_inline() to should_not_inline_for_profile()
>> or something since it is only for C1 compilation with profiling?
>>
> 
> I'd rather leave it generalized if you're not totally against it. We can 
> use it later as a wrapper for oracle to control inlining in both compilers.

OK.

> 
>> I don't understand why you need to recompile caller. It comes from a
>> different frame so it did not inline callee before :
>>
>> + if (max_osr_level == CompLevel_full_optimization) {
>> + // The inlinee OSRed to full opt, we need to modify the enclosing
>> method to avoid deopts
>>
> 
> If we're are in this clause (that is we passed the mh() != imh() check) 
> that means that the event was generated by the imh() that was inlined in 
> mh(). So, we need to recompile mh() because otherwise we would be still 
> executing the inline version of imh() and would be deoptimizing and 
> switching to the full opt version.

Add this as a comment instead of "// If there is an enclosing method"

> 
>> Also next check should be at the beginning of this logic since "Fix up"
>> could trigger unneeded recompilation:
>>
>> + if (!CompileBroker::compilation_is_in_queue(mh, InvocationEntryBci) &&
>> cur_level != next_level) {
> 
> But that would mean that we won't be able to make mh() non-entrant in 
> case mh() is in the queue already. I move the fix up down since the code 
> that makes the enclosing method non-entrant doesn't depend on it.

OK.

Thanks,
Vladimir

> 
> 
> igor
> 
>>
>> Vladimir
>>
>> Igor Veresov wrote:
>>> This allows inlining of methods with loops by C1 on levels with
>>> profiling (2 and 3). The solution is to recompile the enclosing
>>> methods without inlining of the method that has OSRed to level 4 or
>>> recompile the enclosing method at level 4.
>>>
>>> There's no significant impact on performance because of these changes.
>>>
>>> Webrev: http://cr.openjdk.java.net/~iveresov/7057120/webrev.00/
>>>
>>> The change is for 7u2.
>>>
>>> Thanks,
>>> igor
>