[9] RFR(S): 8056071: compiler/whitebox/IsMethodCompilableTest.java fails with 'method() is not compilable after 3 iterations'

Fri Nov 7 07:21:28 UTC 2014

Hi Igor,

thanks for looking at this. See comments inline.

On 06.11.2014 20:47, Igor Ignatyev wrote:
> Hi Tobias,
> 
> On 11/06/2014 01:25 PM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review.
>>
>> On 06.11.2014 01:56, Vladimir Kozlov wrote:
>>> So the fix is add -XX:-TieredCompilation.
>>> The rest is fix problems found due to these failure.
>>
>> Yes, exactly.
>>
>>> May be we should add a new test (other RFE) which would test compilation of
>>> trivial methods in Tiered mode. Current failure shows that we don't test it.
>>
>> I agree. I filed RFE JDK-8056071 and will take care of it.
> you posted wrong cr.
> 
> actually, PavelP is working on such the tests, it's tracked by 8059575 and/or
> 8028590.

Thanks, the right RFE is JDK-8063096. I closed it as a duplicate of JDK-8028590
because it is not directly related to the segmented code cache test plan.

Best,
Tobias

>>
>>> I am not comfortable to rely on data which is set only by C1. This information
>>> is accessed in non Tiered mode too.
>>
>> 'is_trivial' is only used with the Simple and AdvancedThresholdPolicy, i.e., if
>> TieredCompilation is enabled. Where do you think it is used in non-tiered mode?
>>
>>> Why not use method->has_loops(), for example?
>>
>> We also decide based on the number of blocks and 'MethodData::would_profile'
>> which is only available if we have a C1 compiled version.
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 11/5/14 4:12 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> please review the following patch.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8056071
>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8056071/webrev.00/
>>>>
>>>> Problem:
>>>> The test compiles and deoptimizes a method m multiple times. After each
>>>> deoptimization the test checks if m was compiled with C2 and if so increments a
>>>> counter. The test assumes that m is compilable until the counter reaches the
>>>> 'PerMethodRecompilationCutoff'. This does not hold in the following case:
>>>> - The first compilation request for m is added to the compile queue.
>>>> Although we
>>>> profiled m and the MDO suggests that m is trivial, we do not use this
>>>> information because due to deoptimization there is no corresponding nmethod
>>>> [1].
>>>> We compile at level 4 and execution continues in the interpreter [2].
>>>> - A second compilation request for m is issued and while it is being processed
>>>> the first compilation request finishes [3]. Because a nmethod now exists, we
>>>> use
>>>> the MDO and decide to compile at level 1 since m is trivial.
>>>> - The level 4 compiled nmethod is replaced by the new level 1 nmethod [4].
>>>>
>>>> After the following deoptimization the counter is not incremented because the
>>>> nmethod was compiled at level 1. As a result m is set to not compilable before
>>>> the counter reaches the 'PerMethodRecompilationCutoff'. The test fails.
>>>>
>>>> Problems with the current implementation:
>>>> (1) We should always use the MDO if it is valid, even if there is no
>>>> corresponding nmethod. Otherwise we compile trivial methods at level 4 or start
>>>> profiling again.
>>>> (2) If there is a nmethod the MDO may still be invalid: We can compile a method
>>>> immediately at C2, deoptimize, and the MDO is uninitialized. In this case we
>>>> wrongly assume that the method is trivial and re-compile it with C1.
>>>> (3) To avoid a level 2 or 3 compilation and profiling we should mark simple
>>>> constant getter methods, like those used by the test, as trivial.
>>>> (4) We should add verification code to avoid/catch such bad tiered level
>>>> transitions in the future.
>>>>
>>>> Solution:
>>>> To fix (1) and (2) I added a field '_stats_valid' to MethodData that determines
>>>> if '_num_loops' and '_num_blocks' are set (i.e. the method was previously C1
>>>> compiled). Instead of checking for a nmethod, 'is_trivial()' now checks if the
>>>> MDO is valid. I added -XX:-TieredCompilation to the test because otherwise we
>>>> always compile the (trivial) methods at level 1 and the test fails.
>>>>
>>>> To fix (3) I added the method 'Method::is_constant_getter()' that determines if
>>>> a method consists of a constant push and a return statement only. If so, we
>>>> treat the method as trivial. See trivial.log [5] and trivial_fixed.log [6].
>>>>
>>>> I filed JDK-8062913 for (4) because the verification code I added still fails
>>>> with the current implementation due to 2->2 and 3->2 transitions.
>>>>
>>>> Testing:
>>>> - JPRT including failing whitebox test
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>>
>>>> [1] See implementation of 'SimpleThresholdPolicy::is_trivial(..)'
>>>> [2] First compilation request:
>>>> InterpreterRuntime::frequency_counter_overflow
>>>>     InterpreterRuntime::frequency_counter_overflow_inner
>>>>       SimpleThresholdPolicy::event
>>>>         AdvancedThresholdPolicy::method_invocation_event
>>>>           AdvancedThresholdPolicy::call_event
>>>>             AdvancedThresholdPolicy::common
>>>>               [is_trivial() returns false because no nmethod available]
>>>>               [next_level = CompLevel_Full_Optimization]
>>>>           SimpleThresholdPolicy::compile
>>>>             SimpleThresholdPolicy::submit_compile
>>>>               CompileBroker::compile_method
>>>>                 CompileBroker::compile_method_base
>>>>                   CompileBroker::create_compile_task
>>>>     [continue execution in interpreter]
>>>>
>>>> [3] Second compilation request:
>>>> InterpreterRuntime::frequency_counter_overflow
>>>>     InterpreterRuntime::frequency_counter_overflow_inner
>>>>       SimpleThresholdPolicy::event
>>>>         AdvancedThresholdPolicy::method_invocation_event
>>>>         [First compilation finishes here ->
>>>> !CompileBroker::compilation_is_in_queue()]
>>>>         AdvancedThresholdPolicy::call_event
>>>>           AdvancedThresholdPolicy::common
>>>>             [is_trivial() returns true because nmethod/mdo now available]
>>>>             [next_level = CompLevel_Simple]
>>>>     [CompLevel_Simple nmethod replaced CompLevel_Full_Optimization method]
>>>>
>>>> [4] https://bugs.openjdk.java.net/secure/attachment/23321/PrintCompilation.log
>>>> [5] https://bugs.openjdk.java.net/secure/attachment/23327/trivial.log
>>>> [6] https://bugs.openjdk.java.net/secure/attachment/23328/trivial_fixed.log
>>>>