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

Tobias Hartmann tobias.hartmann at oracle.com
Mon Nov 10 08:37:58 UTC 2014


Vladimir, Igor, thanks for the reviews.

On 07.11.2014 17:21, Vladimir Kozlov wrote:
>  You may change behavior in other places where would_profile() is used. Please, verify. Otherwise looks good

Would_profile() is only used by the 'common' methods of the Simple and
AdvancedThresholdPolicies to decide if we need more profiling or should switch
to full optimization.
In the current implementation _would_profile is initialized to false and
therefore would_profile() returns false for methods that were never compiled
with C1. The fixed implementation returns true in this case.

However, the 'common' methods only invoke would_profile() from level 2 and 3,
i.e. if a C1 compiled version exists. There should be no behavioural difference.

>  Thanks,
>  Vladimir

On 08.11.2014 09:23, Igor Veresov wrote:
> In your change _would_profile is still a bool, I guess it should be of the enum
> type above or at least an int?

Thanks! I missed that.

New webrev: http://cr.openjdk.java.net/~thartmann/8056071/webrev.02/

Thanks,
Tobias

> igor
> 
> On Nov 6, 2014, at 11:58 PM, Tobias Hartmann <tobias.hartmann at oracle.com
> <mailto:tobias.hartmann at oracle.com>> wrote:
> 
>> Hi Vladimir,
>>
>> thanks for the review.
>>
>> On 07.11.2014 01:50, Vladimir Kozlov wrote:
>>> 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;
>>>  }
>>
>> Okay, I changed the code accordingly.
>>
>>> 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().
>>
>> I agree. Like this, we also save the additional '_stats_valid' field in
>> MethodData. I added an enum and changed the implementation of 'stats_valid()'.
>> It now returns true if '_would_profile' is not initialized or set to true.
>>
>> New webrev: http://cr.openjdk.java.net/~thartmann/8056071/webrev.01/
>>
>> Thanks,
>> Tobias
>>
>>> 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
> 


More information about the hotspot-compiler-dev mailing list