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

[Vladimir Ivanov]

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
>>>>>
>>>
>