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

[Vladimir Kozlov]

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