RFR: 8262950: Restructure compiler/intrinsics/TestRotate.java for easier compilation

[Aleksey Shipilev]

On Wed, 3 Mar 2021 19:33:24 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> It seems that after [JDK-8256438](https://bugs.openjdk.java.net/browse/JDK-8256438) this test is very slow. See [JDK-8262465 ](https://bugs.openjdk.java.net/browse/JDK-8262465) for more evidence of this.
>> I believe the underlying cause is that `main()` is way too busy with test cases, so this eventually happens:
>> 
>> $ javac test/hotspot/jtreg/compiler/intrinsics/TestRotate.java
>> $ java -XX:-TieredCompilation -XX:CompileThreshold=1000 -Xbatch -XX:+PrintCompilation -cp test/hotspot/jtreg compiler.intrinsics.TestRotate
>>  ...
>>    6112 112 % !b compiler.intrinsics.TestRotate::main @ 1494 (3008 bytes) COMPILE SKIPPED: out of nodes during split (retry at different tier)
>> 
>> So, while the test is intended to test rotate intrinsics, I think it never gets there because compilation bails. And this also makes the test slow. This is tier1 test, and so its performance is somewhat important for developer experience.
>> 
>> This improvement restructures the test to be easier to JIT compile. This drops the test run time from ~4 minutes to ~20 seconds on my desktop. Plus, additional diagnostic flag verifies that compiler does not bail anywhere.
>> 
>> Attn @theRealELiu.
>> 
>> Additional testing:
>>   - [x] Linux x86_64 fastdebug, affected test
>>   - [x] Linux x86_64 release, affected test
>>   - [x] Linux aarch64 fastdebug, affected test
>
> I completely agree with optimizing these tests. But what you did is not enough.
> 
> `ITER = 500,000;` is too much. C2 compilation will be triggered at 10,000. If you want to be completely sure use 50,000 which is more than enough.
> 
> Second, `verify()` should be called only after compilation. Why to call it for Interpreter? You can.
> 
> Third, `ITER` and `*_VALUES` loops should be inverted - hot loop will be inner.
> 
> Fourth, `verify()` error message could be part of `new Error()` message instead of separate print line. It will be ease to see.
> 
> Something like this (indents could be wrong):
>     public static void testRorOrInts() {
>          for (int i = 0; i < INT_VALUES.length; i++) {
>              int val = INT_VALUES[i];
>              verify("testRorOrInt1(" + val + ")",  testRorOrInt1(val),  TEST_ROR_OR_INT_1_EXPECTED[i]);
>              verify("testRorOrInt16(" + val + ")", testRorOrInt16(val), TEST_ROR_OR_INT_16_EXPECTED[i]);
>              verify("testRorOrInt31(" + val + ")", testRorOrInt31(val), TEST_ROR_OR_INT_31_EXPECTED[i]);
>              verify("testRorOrInt32(" + val + ")", testRorOrInt32(val), TEST_ROR_OR_INT_32_EXPECTED[i]);
>              for (int count = 0; count < ITERS; count++) {
>                   testRorOrInt1(val);
>                   testRorOrInt16(val);
>                   testRorOrInt31(val);
>                   testRorOrInt32(val);
>              }
>              verify("testRorOrInt1(" + val + ")",  testRorOrInt1(val),  TEST_ROR_OR_INT_1_EXPECTED[i]);
>              verify("testRorOrInt16(" + val + ")", testRorOrInt16(val), TEST_ROR_OR_INT_16_EXPECTED[i]);
>              verify("testRorOrInt31(" + val + ")", testRorOrInt31(val), TEST_ROR_OR_INT_31_EXPECTED[i]);
>              verify("testRorOrInt32(" + val + ")", testRorOrInt32(val), TEST_ROR_OR_INT_32_EXPECTED[i]);
>          }
>      }

By "not enough" you probably mean "while we are at it, we can do even better"? Because current version definitely helps JIT compilation, as evidenced by test times and the absence of compilation aborts.

Off the top of my head:
 - Dropping `ITERS` one order of magnitude down to 50K does not improve test times, AFAICS.
 - Verifying each on each step verifies OSR compilations as well, which must be good.
 - Having a test call not in triplicate (how you suggest) but in singular (how it is in this PR) seems more readable.

I can take a look at these later, but honestly, I think the current patch is an acceptable thing to make `tier1` run fast again.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2811