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

[Vladimir Kozlov]

On 11/6/14 2:25 AM, 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.
>
>> 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?

You are right. NonTieredCompPolicy::is_mature() does not use it.

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

Yes, but next code will do the same and only checking MDO in the last case:

   bool SimpleThresholdPolicy::is_trivial(Method* method) {
     if (method->is_accessor() ||
         method->is_constant_getter()) {
       return true;
     }
     if (method->has_loops() || (method->code_size() >= 15)) {
       return false;
     }
     MethodData* mdo = method->method_data();
     if (mdo != NULL && mdo->stats_valid() && !mdo->would_profile() &&
         (method->code_size() < 5 || (mdo->num_blocks() < 4))) {
         return true;
     }
     return false;
   }

Also mdo->_would_profile will not be set if C1 compilation is bailed out 
(Compilation() in c1_Compilation.cpp). So we can't trust it too.
May be mdo->_would_profile should be enum {unknown, no_profile, profile} 
and we use it instead of mdo->stats_valid().

Thanks,
Vladimir

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