[9] RFR(S): 8046268: compiler/whitebox/ tests fail : must be osr_compiled

[Tobias Hartmann]

Thank you, Vladimir.

Best,
Tobias

On 08.10.2014 17:37, Vladimir Kozlov wrote:
> webrev.02 looks good to me.
>
> Thanks,
> Vladimir
>
> On 10/8/14 2:53 AM, Tobias Hartmann wrote:
>> Hi Igor,
>>
>> thanks for the review.
>>
>> On 08.10.2014 11:18, Igor Ignatyev wrote:
>>> Tobias,
>>>
>>> On 10/08/2014 12:29 PM, Tobias Hartmann wrote:
>>>> Hi Vladimir,
>>>>
>>>> thanks for the review.
>>>>
>>>> On 03.10.2014 19:20, Vladimir Kozlov wrote:
>>>>> Nightly runs done with -Xcomp. Make sure your changes still work.
>>>>
>>>> I did some more testing on JPRT with -Xcomp and also with
>>>> -XX:-TieredCompilation. No failures (the results are linked in the
>>>> comment section). Most tests explicitly specify '-Xmixed'.
>>>>
>>>>> 200 for tiered may be too high. C1 compilation triggered before that,
>>>>> please check.
>>>>
>>>> Yes, that's intended. After some more testing I even increased the value
>>>> to 2000 to make sure that the loop exit branch probability is high
>>>> enough and we never add an uncommon trap. If the method is compiled
>>>> during warmup the following deoptimization ensures that we throw away
>>>> the compiled version and an OSR compilation is triggered:
>>>>
>>>>   551             // Make sure method is not (yet) compiled
>>>>   552             deoptimize(m);
>>> no, it doesn't since you call 'WhiteBox.getWhiteBox().deoptimizeMethod(e,
>>> false);' where false means non-osr.
>>
>> Yes, that's intended. We want to trigger an OSR compilation after the warmup.
>> Therefore we remove all non-OSR versions
>> by deoptimizing them. If the warmup triggers an OSR compilation (which is
>> really unlikely because the loop is executed
>> for only one iteration each) that's fine as well.
>>
>> I added some comments to make this more explicit.
>>
>>>>
>>>> I also added code to wait for a potential background compilation (see
>>>> 'Helper.deoptimize') because the method may still be in the compile queue.
>>> we already have 'CompilerWhiteBoxTest::waitBackgroundCompilation', I think it'd
>>> be better to add an argument to it and make it static, but leave instance method
>>> w/o args which calls it w/ method.
>>
>> Done. New webrev:
>>
>> http://cr.openjdk.java.net/~thartmann/8046268/webrev.02/
>>
>> Thanks,
>> Tobias
>>
>>>>
>>>> The fixed tests pass on all platforms with and without -Xcomp.
>>>>
>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8046268/webrev.01/
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 10/3/14 2:22 AM, Tobias Hartmann wrote:
>>>>>> Hi,
>>>>>>
>>>>>> please review the following patch.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8046268
>>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8046268/webrev.00/
>>>>>>
>>>>>> == Problem ==
>>>>>> The whitebox tests assume that after the invocation of
>>>>>> 'CompilerWhiteBoxTest.compile()' the method is always compiled
>>>>>> and 'CompilerWhiteBoxTest.checkCompiled()' will return true. For OSR
>>>>>> compilations [1] this is does not hold. For
>>>>>> example, in case of the following events:
>>>>>>
>>>>>> 1) The test invokes 'CompilerWhiteBoxTest.compile()' to OSR compile
>>>>>> 'SimpleTestCase.Helper.osrMethod()'.
>>>>>> 2) OSR compilation is performed at level 4. An uncommon trap is added
>>>>>> after the loop because profiling never reached
>>>>>> that point. Execution continues with the OSR version.
>>>>>> 3) Deoptimization is triggered because the loop finishes (reason:
>>>>>> 'unstable if'). The OSR nmethod is made non-entrant
>>>>>> and removed from the OSR list (see 'nmethod::invalidate_osr_method').
>>>>>> 4) The test uses CompilerWhiteBoxTest.checkCompiled() to make sure the
>>>>>> method is OSR compiled. Because the OSR nmethod
>>>>>> was removed from the list, check compiled returns false (see
>>>>>> WB_IsMethodCompiled).
>>>>>>
>>>>>> This was caused by the fix for JDK-8030976 [2] because we now trust
>>>>>> the profile information more and add uncommon traps
>>>>>> to untaken paths. In this case we add an uncommon trap after the loop
>>>>>> exit:
>>>>>>
>>>>>> private int osrMethod() {
>>>>>>    int result = 0;
>>>>>>    for (long i = 0; i < CompilerWhiteBoxTest.BACKEDGE_THRESHOLD; ++i) {
>>>>>>      result += method();
>>>>>>    }
>>>>>>    return result;
>>>>>> }
>>>>>>
>>>>>> == Solution ==
>>>>>> I added code to 'warm up' the methods before triggering OSR
>>>>>> compilation by executing them a limited number of times.
>>>>>> Like this, the profile information marks the loop exit as taken and we
>>>>>> don't add an uncommon trap.
>>>>>>
>>>>>> == Testing ==
>>>>>> - Executed failing tests on JPRT
>>>>>>
>>>>>> Thanks,
>>>>>> Tobias
>>>>>>
>>>>>>
>>>>>> [1] OSR compiled methods:
>>>>>> compiler.whitebox.SimpleTestCase.Helper.OSR_CONSTRUCTOR
>>>>>> compiler.whitebox.SimpleTestCase.Helper.OSR_METHOD
>>>>>> compiler.whitebox.SimpleTestCase.Helper.OSR_STATIC
>>>>>>
>>>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8030976