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

[Albert]

Thank you for looking at this, Tobias.

Best,
Albert

On 05/08/2014 11:30 AM, Tobias Hartmann wrote:
> Looks good to me.
>
> Regards,
> Tobias
>
> On 08.05.2014 11:07, Albert wrote:
>> Thanks for the review, Vladimir.
>> Do I need a second one?
>>
>> Best,
>> Albert
>>
>> On 05/08/2014 11:00 AM, Vladimir Kozlov wrote:
>>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>
>>
>