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

[Jie Fu]

Thanks Coleen for your review.


On 2019年05月04日 05: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
>>>
>