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

[Albert]

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