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

[Nils Eliasson]

Hi Tobias,

Looks good,

Thanks for fixing this,
Nils
(not a reviewer)

On 2014-11-05 13:12, 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