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

[Albert]

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