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

[Jie Fu]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8221542.patch
Type: text/x-patch
Size: 7109 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20190504/29f0678d/8221542.patch>