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

[Vladimir Kozlov]

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