RFR: 8239423: [TESTBUG] jdk/jfr/jvm/TestJFRIntrinsic.java failed with -XX:-TieredCompilation

[Ao Qi]

On Wed, Feb 19, 2020 at 8:32 PM David Holmes <david.holmes at oracle.com> wrote:
>
> On 19/02/2020 5:40 pm, David Holmes wrote:
> > On 19/02/2020 5:26 pm, Ao Qi wrote:
> >> On Wed, Feb 19, 2020 at 3:01 PM David Holmes <david.holmes at oracle.com>
> >> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 19/02/2020 4:20 pm, Ao Qi wrote:
> >>>> Hi all,
> >>>>
> >>>> Could you please review this patch?
> >>>>
> >>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8239423
> >>>> webrev: http://cr.openjdk.java.net/~aoqi/8239423/webrev.00/ (22 lines
> >>>> changed: 19 ins; 0 del; 3 mod; 153 unchg)
> >>>
> >>> I'm not seeing the connection between the reported bug and the fix. It
> >>> seems to me that passing -XX:-TieredCompilation to a test that
> >>> explicitly sets -XX:TieredStopAtLevel=n is simply not something that can
> >>> work. In fact I'm surprised it doesn't trigger an error when validating
> >>> the command-line args.
> >>
> >> "-XX:-TieredCompilation" in the title is a way to trigger the bug, not
> >> the cause. Sorry for the confusion. The bug also can be triggered by
> >> the jdk configured with --with-jvm-features=-compiler1.
> >>
> >>>
> >>> You fix seems more related to whether C1 and/or C2 are available.
> >>
> >> Yes.
> >
> > I've had a look at this and I'm seeing the problem but I don't yet think
> > this is the right fix. I see some general issues with the ability to use
> > the compiler flags in a way that may not make sense (to me anyway); then
> > I see an issue with the way the test determines the available levels.
> >
> > Looking at the test it determines the available levels as follows:
> > - If tiered is enabled
> >      - use TieredStopAtLevel=n to set the maximum level and thus the
> > available levels
> > - else if server
> >    - only level is 4
> > - else // client, minimal, emulated client
> >    - only level is 1
> >
> > So with -TieredCompilation or a server only build the level is set to 4.
> > But the WhiteBox::compile_method rejects that as an invalid level for
> > the configuration.
> >
> > I don't think the fix is to require C2, in fact I don't see how that
> > fixes the problem anyway if you have a C2 only build without tiered
> > because the test will still use level=4 which the WB rejects:
> >
> > STDOUT:
> > WB error: invalid compilation level 4
> >
> > The test in WB is:
> >
> >    if (comp_level > MIN2((CompLevel) TieredStopAtLevel,
> > CompLevel_highest_tier))
> >
> > but I don't know how either of those values are set when (a) tiered is
> > disabled; or (b) you have a C1 or C2 only build (which should imply
> > tiered is disabled as well).

Thanks very much for your hint, David.

>
> Digging deeper if C2 exists then CompLevel_highest_tier should be set as:
>
> #if defined(COMPILER2)
> CompLevel  CompLevel_highest_tier      = CompLevel_full_optimization;
>
> where:
>
> CompLevel_full_optimization = 4
>
> and from the command-line:
>
> -XX:TieredStopAtLevel=4
>
> and so I don't understand how the above failure can trigger. One of
> those fields must not be what is is expected to be.

I think it is the "-XX:TieredStopAtLevel=1" that trigger the failure.
I was also seeing this issue. Here is a table:

|mode            |cl|tsl|cht|>|
|+tiered(default)|4 |4  |4  |false|
|+tiered(default)|1 |1  |4  |false|
|-tiered         |4 |4  |4  |false|
|-tiered         |4 |1  |4  |*true*|
|c2 only         |4 |4  |4  |false|
|c2 only         |4 |1  |4  |*true*|
|c1 only         |1 |4  |1  |false|
|c1 only         |1 |1  |1  |false|

cl: comp_level(level to set)
tsl: TieredStopAtLevel
cht: CompLevel_highest_tier
">": comp_level > min(TieredStopAtLevel, CompLevel_highest_tier)

If ">" is true, the test failed. I think the combination of "c2
only(comp_level=4/CompLevel_highest_tier=4) and TieredStopAtLevel=1"
should be skipped. I am not sure if we should skip the combination of
"-tiered(comp_level=4/CompLevel_highest_tier=4) and
TieredStopAtLevel=1" or we should set comp_level=1 when -tiered is
used.

I need more time to follow up on this issue.

Cheers,
Ao Qi

>
> David
>
>
> > Cheers,
> > David
> >
> >> Cheers,
> >> Ao Qi
> >>
> >>>
> >>> ??
> >>>
> >>> Cheers,
> >>> David
> >>>
> >>>> Thanks,
> >>>> Ao Qi
> >>>>
> >>