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

[Jie Fu]

Hi Tobias,

Thank you for your review.
I will add the brackets as soon as I come back to my office this Sunday.

I sincerely hope that someone from the runtime-dev can also help to 
review this patch[1].
Thanks a lot.

Best regards,
Jie

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


On 2019年05月02日 17:23, Tobias Hartmann wrote:
> 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
>>