[9] RFR(S): 8056071: compiler/whitebox/IsMethodCompilableTest.java fails with 'method() is not compilable after 3 iterations'
Vladimir Kozlov
vladimir.kozlov at oracle.com
Mon Nov 10 15:44:17 UTC 2014
Thank you, Tobias, for verification.
This looks good.
Thanks,
Vladimir
On 11/10/14 12:37 AM, Tobias Hartmann wrote:
> 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