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

[David Holmes]

On 19/02/2020 11:30 pm, Ao Qi wrote:
> 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.

Ah! Now I see the problem. In WhiteBox we have:

   if (comp_level > MIN2((CompLevel) TieredStopAtLevel, 
CompLevel_highest_tier)) {
     tty->print_cr("WB error: invalid compilation level %d", comp_level);
     return false;
   }

but TieredStopAtLevel should be ignored when tiered compilation is 
disabled, or not present. I actually think it is a bug that the VM 
allows you to set TieredStopAtLevel even though it is ignored on those 
configurations. The WB code should be changed to something like:

#ifdef TIERED
   CompLevel cl_min = MIN2((CompLevel) TieredStopAtLevel, 
CompLevel_highest_tier);
#else
   CompLevel cl_min = CompLevel_highest_tier;
#endif
   if (comp_level > cl_min) {
     tty->print_cr("WB error: invalid compilation level %d", comp_level);
     return false;
   }

There are other places in WhiteBox that need similar fixes.

Cheers,
David

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