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

[Vladimir Kozlov]

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