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

[Jie Fu]

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