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

[Vladimir Ivanov]

> Updated: http://cr.openjdk.java.net/~jiefu/8224162/webrev.04/
> 
> For time and safety reasons, I still prefer the one-line fix.
> I think "profile.count < -1" is good enough to catch the overflow and no 
> other action is required for this issue.
> 
> The overflow problem seems to be there for quite a long time.
> It requires much effort and time to fix it correctly and completely, and 
> the risk seems a little high.

(Just noticed that the bug is classified as P1. IMO it should be 
reassessed as P3 and I left a comment there asking for that.)

I'm still in favor of fixing the root cause than putting band-aids in 
otherwise perfectly valid code.

I don't consider fixing CounterData::count() and its usages to properly 
handle overflow as overly complicated. There's a limited number of 
usages and they don't properly handle overflow as well. So, fixing the 
bug is highly desireable even though it has been left unnoticed for a 
long time.

Best regards,
Vladimir Ivanov

> To finish this issue ASAP, it might be better to handle the overflow 
> problem in a separate bug ID.
> What do you think?

> On 2019/5/22 下午11:58, Vladimir Ivanov wrote:
>> Nice catch, Jie!
>>
>> I'm in favor of fixing the overflow instead and keep counts always 
>> non-negative. The root cause for negative counts is uint->int 
>> conversion and values can be normalized to max_int.
>>
>> Moreover, on 64-bit platforms it's easy to catch (and fix) uint 
>> overflow since all the info is there - counts already are 64-bit 
>> (intptr_t).
>>
>> Regarding the test:
>>
>> test/hotspot/jtreg/compiler/profiling/TestProfileCounterOverflow.java:
>>
>>   28  * @requires (vm.debug == true)
>>
>> The test runs just fine with product binaries. So, I'd prefer to avoid 
>> limiting it only to debug.
>>
>>
>>   51         } else {
>>   52             // Sleep to wait for the compilation to be finished
>>   53             Thread.sleep(2000);
>>   54         }
>>
>> You can just turn off background compilation instead (-Xbatch).
>>
>>
>> I was able to significantly speed up the test with the following chnages:
>>
>> /**
>>  * @test
>>  * @run main/othervm -Xbatch -XX:-UseOnStackReplacement 
>> -XX:MaxTrivialSize=0 compiler.profiling.TestProfileCounterOverflow
>>  */
>>
>> package compiler.profiling;
>>
>> public class TestProfileCounterOverflow {
>>     public static void test(long iterations) throws Exception {
>>         for (long j = 0; j < iterations; j++) {
>>             call();
>>         }
>>     }
>>     public static void call() {}
>>
>>     public static void main(String[] args) throws Exception {
>>         // trigger profiling on tier3
>>         for (int i = 0; i < 500; i++) {
>>             test(1);
>>         }
>>
>>         test(Integer.MAX_VALUE + 10000L); // overflow call counter
>>
>>         // trigger c2 compilation
>>         for (int i = 0; i < 10_000; i++) {
>>             test(1);
>>         }
>>         System.out.println("TEST PASSED");
>>     }
>> }
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 22/05/2019 11:55, Jie Fu wrote:
>>> On 2019/5/22 上午11:57, Leonid Mesnik wrote:
>>>> Could you please add reproducer as regression test for fix. Please 
>>>> verify that test failed without fix and pass with fix.
>>>
>>> Done.
>>>
>>> Updated: http://cr.openjdk.java.net/~jiefu/8224162/webrev.03/
>>>
>>> # Testing
>>> On an i7-8700 at 3.20GHz machine, the test results:
>>> -----------------------------------------------
>>> - fastdebug: make test 
>>> TEST="test/hotspot/jtreg/compiler/profiling/TestProfileCounterOverflow.java" 
>>> CONF=fastdebug
>>>    Before fix: failed, elapsed time: 37s
>>>    After  fix: pass,   elapsed time: 43s
>>> - slowdebug: make test 
>>> TEST="test/hotspot/jtreg/compiler/profiling/TestProfileCounterOverflow.java" 
>>> CONF=slowdebug
>>>    Before fix: failed, elapsed time: 38s
>>>    After  fix: pass,   elapsed time: 43s
>>> -----------------------------------------------
>>>
>>> Please review it and give me some advice.
>>>
>>> Thanks.
>>> Best regards,
>>> Jie
>>>
>>>>
>>>> Leonid
>>>>
>>>>> On May 21, 2019, at 8:54 PM, Jie Fu <fujie at loongson.cn> wrote:
>>>>>
>>>>> On 2019/5/21 下午10:30, Vladimir Ivanov wrote:
>>>>>> It doesn't explain the failure since the assert is hit while 
>>>>>> parsing invoke* bytecode while type check failures are recorded 
>>>>>> for checkcast, aastore, and instanceof. Am I missing something 
>>>>>> important here?
>>>>>>
>>>>> Good question. I'm so sorry to make you confused.
>>>>>
>>>>> After a long time of digging into the code, I think this failure 
>>>>> was cause by the overflow of profile.cout.
>>>>>
>>>>> A reproducer is constructed here:
>>>>>   - http://cr.openjdk.java.net/~jiefu/8224162/CounterOverflow.java
>>>>>
>>>>> And I've changed the comment for the patch:
>>>>>   - http://cr.openjdk.java.net/~jiefu/8224162/webrev.02/
>>>>>
>>>>> Please review it and give me some advice.
>>>>>
>>>>> Thanks.
>>>>> Best regards,
>>>>> Jie
>>>>>
>>>>>
>>>
>