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

[Vladimir Kozlov]

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