RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

[Andrew Dinn]

On Fri, 18 Sep 2020 15:55:52 GMT, Jason Tatton <github.com+70893615+jasontatton-aws at openjdk.org> wrote:

>> Hi Andrew,
>> 
>> Thanks for coming back to me. Looking on the github [PR](https://github.com/openjdk/jdk/pull/71) nobody is tagged as a
>> reviewer for this (perhaps this is a feature which is not being used).
>>> That's
>>> because what is actually missing is the justification he asked for. As
>>> Andrew pointed out the change is simple but the reason for implementing
>>> it is not.
>> 
>> There are two separate things here:
>> 1). Justification for the change itself:
>> -The objective and justification for this patch is to bring the performance of StringLatin1 indexOf(char) in line with
>>  StringUTF16 indexOf(char) for x86 and ARM64. This solves the problem as raised in
>>  [JDK-8173585](https://bugs.openjdk.java.net/browse/JDK-8173585), and also on the [mailing
>>  list](http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-January/005539.html).
>> 
>> 2). Discussion around future enhancements - concerning potential use of AVX-512 instructions and a more optimal
>> implementation for short strings.
>> -These would be separate JBS's I'm not advocating for/against this, they are just ideas separate from this JBS.
>
> The AVX2 code path represents approximately 1/6th of the patch (1/7th including the infrastructure  code around this).
> I don’t think we should discard the entire patch because 1/6th of the code may have unintended consequences. This is
> especially the case when the rest of the code has been benchmarked, with certainty, to show the desired performance
> improvement has been achieved.   Additionally, I do not see how those unintended consequences will ever be realised
> because the JVM has knowledge of the AVX capability of the chip it’s running on and disables the AVX2 code path for
> chips which suffer from the performance degradation which has been outlined in this discussion. Thus protecting us from
> unintended consequences. Unless we are asserting that this mechanism to globally control the use of AVX2 instructions
> is broken or otherwise non functional I see no reason to remove the AVX2 code. And to be consistent we would really
> need to look at removing all instances of AVX2 code in the JVM (of which there is quite a lot).   As I see it there are
> three ways forward:  1.  Accept the patch as is.  2. Modify the patch to remove the AVX code path for x86, and/or any
> other modifications needed. 3. Discard the patch entirely.  At this point I am in favour of approach 1 but happy to
> accept 2 if advised that this is the right thing to do.

"the JVM has knowledge of the AVX capability of the chip it’s running on and disables the AVX2 code path for chips
which suffer from the performance degradation which has been outlined in this discussion"

Does it? The white paper Andrew cited doesn't mention this as being specific to only some chips that implement AVX2.
Can you explain where this restricted effect is documented?

Also, I assume you are referring to the code in vm_version_x86.cpp with this comment

    // Don't use AVX-512 on older Skylakes unless explicitly requested

is that correct?

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

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