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

[Jie Fu]

Thank you so much Vladimir Ivanov.

On 2019/5/7 上午3:19, Vladimir Ivanov wrote:
> Sure, pushed [1]
>
> Best regards,
> Vladimir Ivanov
>
> [1] http://hg.openjdk.java.net/jdk/jdk/rev/1abca1170080
>
> On 03/05/2019 16:24, Jie Fu wrote:
>> Hi Vladimir Ivanov,
>>
>> The patch in the attachment has been updated by adding brackets to 
>> the checks in InlineTree::is_not_reached.
>>
>> Is it OK to be pushed?
>> If so, could you please sponsor it?
>>
>> Thanks a lot.
>>
>> Best regards,
>> Jie
>>
>>
>> On 2019年05月04日 06:06, Vladimir Ivanov wrote:
>>> CCing Jie Fu.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 03/05/2019 14:55, coleen.phillimore at oracle.com wrote:
>>>>
>>>> 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
>>>>>>
>>>>
>>