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

[Severin Gehwolf]

Hi,

On Tue, 2014-05-06 at 11:07 -0700, 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.

This information would be nice to have documented in
get_min_number_of_compiler_threads(). In particular, that
CI_COMPILER_COUNT is the *default* and the minimum value may be
different (but >= the default). My $0.02.

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

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