RFR: 8320448: Accelerate IndexOf using AVX2 [v19]

[Scott Gibbons]

On Wed, 15 May 2024 19:34:40 GMT, Volodymyr Paprotski <duke at openjdk.org> wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Rearrange; add lambdas for clarity
>
> test/jdk/java/lang/StringBuffer/IndexOf.java line 90:
> 
>> 88: 
>> 89:   // printStringBytes(shs.getBytes(hs_charset));
>> 90:   for (int i = 0; i < 200000; i++) {
> 
> This wont be a deterministic way to reach the intrinsic. I would suggest copying the idea from test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/unittest/Poly1305UnitTestDriver.java
> 
> i.e. Have two `@run main` invocations at the top of this file, one with default parameters, one with  `-Xcomp -XX:-TieredCompilation`. You dont need a 'driver' program, that was to handle something else.
> 
> 
> /*
>  * @test
>  * @modules java.base/com.sun.crypto.provider
>  * @run main java.base/com.sun.crypto.provider.Poly1305KAT
>  * @summary Unit test for com.sun.crypto.provider.Poly1305.
>  */
> 
> /*
>  * @test
>  * @modules java.base/com.sun.crypto.provider
>  * @summary Unit test for IntrinsicCandidate in com.sun.crypto.provider.Poly1305.
>  * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions -XX:+ForceUnreachable java.base/com.sun.crypto.provider.Poly1305KAT
>  */

Done.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 126:
> 
>> 124:     int aNewLength = getRandomIndex(min, max);
>> 125:     for (int y = 0; y < aNewLength; y++) {
>> 126:       int achar = generator.nextInt(30) + 30;
> 
> This will only ever generate LL cases, i.e. chars from [30,60]. Could be parametrized to also produce utf16 if instead of 30, offset was in the unicode range

Original code.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 199:
> 
>> 197:                 System.out.println("Source="+sourceString.substring(hsBegin, hsBegin + haystackLen));
>> 198:                 System.out.println("Target="+targetString.substring(nBegin, nBegin + needleLen));
>> 199:                 System.out.println("haystackLen="+haystackLen+" neeldeLen="+needleLen+" hsBegin="+hsBegin+" nBegin="+nBegin+
> 
> This looks like 'development scaffolding' (i.e. printf debugging) that was meant to be removed

This is additional information printed upon failure instead of just saying "failed"

> test/jdk/java/lang/StringBuffer/IndexOf.java line 295:
> 
>> 293:         sourceString = generateTestString(99, 100);
>> 294:         sourceBuffer = new StringBuffer(sourceString);
>> 295:         targetString = generateTestString(10, 11);
> 
> Generate a random int [0,1,2] for LL, UU, UL, pass that as parameter to generateTestString() to test the other paths. Same for other tests in this file using this pattern.
> 
> This test is specific to haystacklen=100, needlelen=10..  what about other haystack/needle sizes to exercise all the paths in the intrinsic assembler (i.e. haystack >=, <=32, needlelen ={1,2,3,4,5..32..}). Elsewhere already?

Original code.

> test/jdk/java/lang/StringBuffer/IndexOf.java line 360:
> 
>> 358:         System.err.println("  sAnswer = " + sAnswer + ", sbAnswer = " + sbAnswer);
>> 359:         System.err.println("  testString = '" + testString + "'");
>> 360:         System.err.println("  testBuffer = '" + testBuffer + "'");
> 
> tracing left here and further down

Adding more information on failure.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613915508
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613919180
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613920449
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613922554
PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1613923075