[9] RFR(S): 8042431: compiler/7200264/TestIntVect.java fails with: Test Failed: AddVI 0 < 4

[Tobias Hartmann]

Looks good to me.

Regards,
Tobias

On 08.05.2014 11:07, Albert wrote:
> Thanks for the review, Vladimir.
> Do I need a second one?
>
> Best,
> Albert
>
> On 05/08/2014 11:00 AM, Vladimir Kozlov wrote:
>> Excellent.
>>
>> Thanks,
>> Vladimir
>>
>> On 5/7/14 10:34 PM, Albert wrote:
>>> Hi Vladimir,
>>>
>>> that sounds like a good suggestion to me. Here is the updated webrev:
>>> http://cr.openjdk.java.net/~anoll/8042431/webrev.03/
>>>
>>> Thanks for looking at this again.
>>>
>>> Best,
>>> Albert
>>>
>>> On 05/07/2014 07:08 PM, Vladimir Kozlov wrote:
>>>> I just saw Severin's comment.
>>>>
>>>> The check we have already verifies that minimum <= 
>>>> CI_COMPILER_COUNT when CICompilerCount is not specified on command
>>>> line. But to have explicit assert is better, I think:
>>>>
>>>>   int min_number_of_compiler_threads = 
>>>> get_min_number_of_compiler_threads();
>>>>   // The default CICompilerCount's value is CI_COMPILER_COUNT.
>>>>   assert(min_number_of_compiler_threads <= CI_COMPILER_COUNT, 
>>>> "minimum should be less or equal default number");
>>>>   // Check the minimum number of compiler threads.
>>>>   status &=verify_min_value(CICompilerCount, 
>>>> min_number_of_compiler_threads, "CICompilerCount");
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 5/7/14 9:04 AM, Vladimir Kozlov wrote:
>>>>> Good.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 5/7/14 8:08 AM, Albert wrote:
>>>>>> Hi Vladimir,
>>>>>>
>>>>>> thanks for looking at this. I followed your suggestions and 
>>>>>> adapted the webrev accordingly.
>>>>>> Here is the new version:
>>>>>> http://cr.openjdk.java.net/~anoll/8042431/webrev.02/
>>>>>>
>>>>>> Best,
>>>>>> Albert
>>>>>>
>>>>>> On 05/06/2014 08:07 PM, Vladimir Kozlov wrote:
>>>>>>> The default for C2 non-tiered should be 2, not 1.
>>>>>>>
>>>>>>> We are mixing 2 things: minimum value and default value. The 
>>>>>>> default is CI_COMPILER_COUNT.
>>>>>>>
>>>>>>> Next statement is wrong for ZERO VM:
>>>>>>> "A value of 2 works for any platform/configuration."
>>>>>>>
>>>>>>> I don't think we should change globals.hpp, leave it as it is.
>>>>>>>
>>>>>>> We only need to fix the minimum check. The bug is that we used 
>>>>>>> default CI_COMPILER_COUNT as minimum value which is
>>>>>>> wrong.
>>>>>>>
>>>>>>> The fix should be:
>>>>>>>
>>>>>>> +   // Check the minimum number of compiler threads
>>>>>>> +   status &=verify_min_value(CICompilerCount, 
>>>>>>> get_min_number_of_compiler_threads(), "CICompilerCount");
>>>>>>>
>>>>>>> Note, don't reset the default setting.
>>>>>>>
>>>>>>> You can reduce size of new method more:
>>>>>>>
>>>>>>> + int Arguments::get_min_number_of_compiler_threads() {
>>>>>>> + #if !defined(COMPILER1) && !defined(COMPILER2) && !defined(SHARK)
>>>>>>> +   return 0;   // case 1: no compilers (Zero VM)
>>>>>>> + #else
>>>>>>> +   if (!TieredCompilation || (TieredStopAtLevel < 
>>>>>>> CompLevel_full_optimization)) {
>>>>>>> +     return 1; // case 2 or case 3
>>>>>>> +   }
>>>>>>> +   return 2;   // case 4 (tiered).
>>>>>>> + #endif
>>>>>>> + }
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 5/6/14 6:33 AM, Albert wrote:
>>>>>>>> Hi Niclas,
>>>>>>>>
>>>>>>>> thanks for your suggestions. Your version is much nicer. Here 
>>>>>>>> is the
>>>>>>>> updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~anoll/8042431/webrev.01/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Albert
>>>>>>>>
>>>>>>>> On 05/06/2014 03:25 PM, Niclas Adlertz wrote:
>>>>>>>>> Hi, see my previous answer. If would suggest to change the nested
>>>>>>>>> if-else cases to:
>>>>>>>>>
>>>>>>>>> // case 1
>>>>>>>>> if (no_compiler) {
>>>>>>>>>   return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> // case 2 or 3
>>>>>>>>> if (!TieredCompilation || TieredStopAtLevel <
>>>>>>>>> CompLevel_full_optimization) {
>>>>>>>>>   return 1;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> // case 4 (default case, works on all platforms)
>>>>>>>>> return 2
>>>>>>>>>
>>>>>>>>> Kind Regards,
>>>>>>>>> Niclas Adlertz
>>>>>>>>>
>>>>>>>>> On 05/06/2014 03:21 PM, Albert wrote:
>>>>>>>>>> Hi Niclas,
>>>>>>>>>>
>>>>>>>>>> thanks for looking at this. Please see comment inline.
>>>>>>>>>> On 05/06/2014 03:04 PM, Niclas Adlertz wrote:
>>>>>>>>>>> Hi Albert,
>>>>>>>>>>>
>>>>>>>>>>> Inside src/share/vm/runtime/arguments.cpp:
>>>>>>>>>>> +   // We should not come here. However, we also do not fail 
>>>>>>>>>>> the VM,
>>>>>>>>>>> since this is a not-critical error.
>>>>>>>>>>> +   assert (false, "Missed configuration to set the minimum 
>>>>>>>>>>> number of
>>>>>>>>>>> compiler threads.");
>>>>>>>>>>> +   // Return 2, since this works with all configurations on 
>>>>>>>>>>> all
>>>>>>>>>>> platforms.
>>>>>>>>>>> +   return 2;
>>>>>>>>>>>
>>>>>>>>>>> That is dead code, and we can remove it.
>>>>>>>>>>> If none of the cases 1, 2 or 3 are true, we will always 
>>>>>>>>>>> enter case 4.
>>>>>>>>>> That code is there to make sure (in case someone makes 
>>>>>>>>>> changes to that
>>>>>>>>>> function)
>>>>>>>>>> that the default return value is 2, since this works for all 
>>>>>>>>>> platforms.
>>>>>>>>>>
>>>>>>>>>> Do you think that leaving a comment is good enough (or not 
>>>>>>>>>> even that)?
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Kind Regards,
>>>>>>>>>>> Niclas Adlertz
>>>>>>>>>>>
>>>>>>>>>>> On 05/06/2014 02:43 PM, Albert wrote:
>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>
>>>>>>>>>>>> could I get reviews for this patch?
>>>>>>>>>>>>
>>>>>>>>>>>> Bug:
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8042431
>>>>>>>>>>>>
>>>>>>>>>>>> Problem:
>>>>>>>>>>>> The minimum number of compiler threads is set incorrectly.
>>>>>>>>>>>>
>>>>>>>>>>>> Solution:
>>>>>>>>>>>> Define a new function 
>>>>>>>>>>>> 'get_min_number_of_compiler_threads()' that
>>>>>>>>>>>> determines the minimum number of compiler threads for a 
>>>>>>>>>>>> particular
>>>>>>>>>>>> platform/configuration.
>>>>>>>>>>>>
>>>>>>>>>>>> Testing:
>>>>>>>>>>>> Failing test case; zero; -XX:-TieredCompilation 
>>>>>>>>>>>> -XX:CICompilerCount=1
>>>>>>>>>>>>
>>>>>>>>>>>> Webrev:
>>>>>>>>>>>> http://cr.openjdk.java.net/~anoll/8042431/webrev.00/
>>>>>>>>>>>>
>>>>>>>>>>>> Many thanks in advance,
>>>>>>>>>>>> Albert
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>