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

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~vlivanov/jiefu/8221542/webrev.02/src/hotspot/share/oops/cpCache.cpp.frames.html

This looks like it should have gotten the wrong answer without this 
change (there appears to be protection from an index out of range) even 
without your patch.  f2 is Method* for invokeinterface now.

The runtime part of this change look good to me.

Thanks,
Coleen


On 5/2/19 5:23 AM, 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
>>