RFR: 8224162: assert(profile.count() == 0) failed: sanity in InlineTree::is_not_reached

[Jie Fu]

Hi Vladimir Ivanov,

Thanks for your great idea.
With your help, the overflow problem had been fixed both on 64- & 32-bit 
platforms.
Please review it: http://cr.openjdk.java.net/~jiefu/8224162/webrev.09/

Thanks a lot.
Best regards,
Jie

On 2019/5/31 下午6:02, Vladimir Ivanov wrote:
> Thanks for checking 32-bit port!
>
> I'd try to fix the overflow in ciMethod::call_profile_at_bci() by 
> enforcing the following property:
>   // The call site count is 0 with known morphism (only 1 or 2 receivers)
>   // or < 0 in the case of a type check failure for checkcast, 
> aastore, instanceof.
>   // The call site count is > 0 in the case of a polymorphic virtual 
> call.
>
> It knows the bci and has access to the actual bytecode.
>
> For invoke* it would turn negative values into max_jint and for 
> checkcast/aastore/instanceof turn positive values into min_jint.
>
> Also, I would avoid "#ifndef _LP64" and keep the code on both 64- & 
> 32-bit, though on 64-bit it should be effectively unused (as of now, 
> but not necessarily in the future).
>
> Best regards,
> Vladimir Ivanov
>
> On 31/05/2019 12:41, Jie Fu wrote:
>> Hi Vladimir Ivanov,
>>
>> The previous version still failed on 32-bit VM.
>>
>> Updated: http://cr.openjdk.java.net/~jiefu/8224162/webrev.08/
>>
>> There seems no way to detect the invoke-profile counter overflow in 
>> CounterData::count() on 32-bit systems.
>> So I did that in Compile::call_generator(...) for 32-bit platforms.
>>
>> What do you think of this version?
>>
>> Thanks a lot.
>> Best regards,
>> Jie
>>
>> On 2019/5/31 下午3:55, Jie Fu wrote:
>>> Hi Vladimir Ivanov,
>>>
>>> I'm wondering whether the patch[1] works for 32-bit JVM.
>>> And now I'm doing experiments on a 32-bit system.
>>> Please just wait for me and I'll let you know the result ASAP.
>>>
>>> Thanks a lot.
>>> Best regards,
>>> Jie
>>>
>>> [1] http://cr.openjdk.java.net/~jiefu/8224162/webrev.07/
>>>
>>>
>>> On 2019/5/31 下午1:03, Jie Fu wrote:
>>>> Hi Vladimir Ivanov,
>>>>
>>>> Thank you for your review and guidance.
>>>> I benefit a lot from the discussion with you.
>>>> The patch had been updated based on your suggestions:
>>>> - http://cr.openjdk.java.net/~jiefu/8224162/webrev.07/
>>>>
>>>> Also I had changed the parameter type of 
>>>> CounterData::set_count(...) from uint to int.
>>>> It is only used here[1], which I think is safe and clearer to do that.
>>>> Please review it and give me some advice.
>>>>
>>>> Testing:
>>>>   make test TEST="tier1 tier2 tier3" JTREG="JOBS=4" CONF=release on 
>>>> Linux/x64
>>>>
>>>> Thanks a lot.
>>>> Best regards,
>>>> Jie
>>>>
>>>> [1] 
>>>> http://hg.openjdk.java.net/jdk/jdk/file/b0513c833960/src/hotspot/share/oops/methodData.hpp#l1170 
>>>>
>>>>
>>>>
>>>> On 2019/5/30 下午10:59, Vladimir Ivanov wrote:
>>>>> Switching return type to int would make it clearer.
>>>>>
>>>> Done
>>>>
>>>>
>>>>>
>>>>> call->receiver_count(i) is uint, so you still can experience 
>>>>> overflow when converting from uint to int. Considering receiver 
>>>>> counts are positive, I'd use saturating add on uints (and don't 
>>>>> care about min_jint).
>>>>
>>>> Fixed.
>>>>
>>>>
>>>>> Regarding handling overflow during profiling:
>>>>>
>>>>>   * C1 doesn't handle counter overflow [1]
>>>>>
>>>>>   * What template interpreter does to avoid overflow is not enough 
>>>>> for concurrent case: it stores new value into memory and then 
>>>>> conditionally decrements it, but another thread may already see 
>>>>> it. Proper solution would be to keep the value in register, but 
>>>>> that requires a temporary register to burn.
>>>>
>>>> Nice catch! I didn't realize the problem before. Thanks.
>>>>
>>>>
>>>>> So, it seems easier and cheaper (from the perspective of profiling 
>>>>> overhead) to handle that on compiler side when interpreting the data.
>>>>>
>>>> I agree.
>>>>
>>>>
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> [1] 
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/c41783eb76eb/src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp#l1617 
>>>>>
>>