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

[Vladimir Ivanov]

> Please review it: http://cr.openjdk.java.net/~jiefu/8224162/webrev.09/

Looks good! I'll submit it for testing.

Some nitpicking:

src/hotspot/share/ci/ciMethod.cpp

+template <typename L, typename R>
+int ciMethod::saturated_add(L a, R b) {

What do you think about moving it to globalDefinitions.hpp?

Probably, it's worth getting rid of the template parameters and 
introduce specializations (akin to JAVA_INTEGER_OP) since the 
implementation makes sense only for int/uint.


+  jlong sum  = src1 + src2;

I'd prefer to see explicit casts to jlong to stress there's no overflow 
possible here for 32-bit values.


+    return c < 0 ? max_jint : c;

+      case Bytecodes::_instanceof: return c > 0 ? min_jint : c;

Please, put parentheses around ternary expressions.


Best regards,
Vladimir Ivanov

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