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

[Tobias Hartmann]

Hi Jie,

this looks good to me too but please add brackets to the checks in InlineTree::is_not_reached.

I've submitted some extended testing and let you know once it passed.

Someone from the runtime team should also have a look at this because your changes affect the
interpreter. CC'ing runtime-dev.

Thanks,
Tobias

On 29.04.19 15:43, Jie Fu wrote:
> 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
>>
> 
>