[9] RFR(S): 8056071: compiler/whitebox/IsMethodCompilableTest.java fails with 'method() is not compilable after 3 iterations'
Tobias Hartmann
tobias.hartmann at oracle.com
Tue Nov 11 09:07:17 UTC 2014
Thanks, Igor.
Best,
Tobias
On 10.11.2014 19:58, Igor Veresov wrote:
> That looks good.
>
> igor
>
>> On Nov 10, 2014, at 12:37 AM, Tobias Hartmann <tobias.hartmann at oracle.com> 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