review(M): 7057120: Tiered: Allow C1 to inline methods with loops
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Jun 22 15:08:53 PDT 2011
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
>
More information about the hotspot-compiler-dev
mailing list