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

[Albert]

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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>