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

[Vladimir Kozlov]

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