RFR: 8221542: ~15% performance degradation due to less optimized inline decision

[Jie Fu]

Hi all,

May I have another review for this change [1] to finalize the fix?
Thanks a lot.

Best regards,
Jie

[1] http://cr.openjdk.java.net/~vlivanov/jiefu/8221542/webrev.02/


On 2019年04月20日 11:35, Jie Fu wrote:
> Ah, I got it.
> I like your patch and benefit a lot from you.
> Thank you so much, Vladimir.
>
> Any comments from other reviewers?
> Thanks.
>
> Best regards,
> Jie
>
> On 2019/4/20 上午11:18, Vladimir Ivanov wrote:
>>
>>>> After some explorations I decided to keep original behavior for 
>>>> immature profiles (profile.count == -1).
>>>
>>> I agree.
>>>
>>> I have two questions here.
>>>
>>> 1. What's the difference of the following two if statements?
>>> -------------------------------------------------
>>> +  if (!callee_method->was_executed_more_than(0))  return true; // 
>>> callee was never executed
>>> +
>>> +  if (caller_method->is_not_reached(caller_bci))  return true; // 
>>> call site not resolved
>>> -------------------------------------------------
>>> I think only one of them is needed.
>>
>> The checks are complimentary: one inspects callee and the other looks 
>> at call site.
>>
>> "!callee_method->was_executed_more_than(0)" ensures that callee was 
>> executed at least once.
>>
>> "caller_method->is_not_reached(caller_bci)" inspects the state of the 
>> call site. If corresponding CP entry is not resolved, then the call 
>> site isn't reached. If is_not_reached() returns false, it's not a 
>> definitive answer: there's still a chance the site is not reached - 
>> consider the case of virtual calls where callee_method may differ for 
>> the same resolved method.
>>
>>> 2. Does the assert in InlineTree::is_not_reached(...) make sense?
>>> Since we have
>>> -------------------------------------------------
>>> if (profile.count() > 0)   return false; // reachable according to 
>>> profile
>>> -------------------------------------------------
>>> and
>>> -------------------------------------------------
>>> if (profile.count() == -1) {...}
>>> -------------------------------------------------
>>> before
>>> -------------------------------------------------
>>> assert(profile.count() == 0, "sanity");
>>> -------------------------------------------------
>>> is the assert redundant?
>>
>> Asserts are intended to be redundant :-) But still catch bugs from 
>> time to time.
>>
>> This one, in particular, checks invariant on profile.count() >= -1 
>> (which is not very useful by itself), but also stresses that 
>> "profile.count() == 0" case is being processed.
>>
>> Best regards,
>> Vladimir Ivanov
>