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

[Vladimir Ivanov]

Thanks for clarifications, Jie!

src/hotspot/share/oops/methodData.hpp:
    // Direct accessor
    uint count() const {
-    return uint_at(count_off);
+    intptr_t raw_data = intptr_at(count_off);
+    if (raw_data > max_jint) {
+      raw_data = max_jint;
+    } else if (raw_data < min_jint) {
+      raw_data = min_jint;
+    }
+    return uint(raw_data);
    }

I agree with this change.
Switching return type to int would make it clearer.


src/hotspot/share/ci/ciMethod.cpp:

+int ciMethod::saturated_add_int(int a, int b) {

-          int rcount = call->receiver_count(i) + epsilon;
+          int rcount = saturated_add_int(call->receiver_count(i), epsilon);

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

Otherwise, the fix looks good.

Inaccuracies in profiling data are expected, but overflows may cause 
drastic changes in behavior, so it's better to catch such cases and 
handle them properly.

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.

So, it seems easier and cheaper (from the perspective of profiling 
overhead) to handle that on compiler side when interpreting the data.


Best regards,
Vladimir Ivanov

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/c41783eb76eb/src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp#l1617


On 30/05/2019 09:16, Jie Fu wrote:
> Hi Vladimir Ivanov,
> 
> Thank you for your kind review and nice suggestions.
> Updated: http://cr.openjdk.java.net/~jiefu/8224162/webrev.06/
> 
> Please see comments inlined below.
> 
> Thanks a lot.
> Best regards,
> Jie
> 
> On 2019/5/29 下午8:02, Vladimir Ivanov wrote:
>> There are other places where overflow can occur:
>>
>>           int rcount = call->receiver_count(i) + epsilon;
>>
>>           receivers_count_total += rcount;
>>
>> Maybe introduce a helper routine for saturating addition?
>>
> Good catch. Fixed.
>>
>> src/hotspot/share/oops/methodData.hpp:
>>
>>    uint count() const {
>> -    return uint_at(count_off);
>> +    intptr_t raw_data = intptr_at(count_off);
>> +    if (raw_data > max_jint) {
>> +      raw_data = max_jint;
>> +    } else if (raw_data < min_jint) {
>> +      raw_data = min_jint;
>> +    }
>> +    return uint(raw_data);
>>    }
>>
>> Should uintptr_t be used here instead? It looks like this version 
>> won't work as expected on 32-bit platforms (since uint => intrptr_t 
>> conversion becomes lossy).
> uintptr_t shouldn't be used here since the counter may contain a 
> negative value[1][2].
> If uintx was used, max_jint would be returned by CounterData::count() in 
> cases of negative values, which is unexpected.
> And I don't think the type conversion is problematic due to the 
> inaccuracy of profile[3].
> What we care about is just whether the counter is greater or less than a 
> specific threshold, not its absolute value.
>>
>> Also, I don't understand why you added comparison against min_jint. 
>> Since counts are unsigned, the following should be enough:
> I did it because the counter may contain a negative value.
> It might be better to also detect and handle the underflow condition.
> The interpreter also does this here[4].
> 
> [1] 
> http://hg.openjdk.java.net/jdk/jdk/file/c41783eb76eb/src/hotspot/share/ci/ciMethod.cpp#l515 
> 
> [2] 
> http://hg.openjdk.java.net/jdk/jdk/file/c41783eb76eb/src/hotspot/share/ci/ciMethod.cpp#l533 
> 
> [3] 
> http://hg.openjdk.java.net/jdk/jdk/file/c41783eb76eb/src/hotspot/share/oops/methodData.hpp#l49 
> 
> [4] 
> http://hg.openjdk.java.net/jdk/jdk/file/c41783eb76eb/src/hotspot/cpu/x86/interp_masm_x86.cpp#l1407 
> 
> 
> 
>>
>>   uint count() const {
>>     uintx raw_data = (uintx)intptr_at(count_off);
>>     if (raw_data > max_jint) {
>>       raw_data = max_jint;
>>     }
>>     return uint(raw_data);
>>   }
>