RFR: 8173585: Intrinsify StringLatin1.indexOf(char)

[Jason Tatton]

On Thu, 17 Sep 2020 18:20:08 GMT, Jason Tatton <github.com+70893615+jasontatton-aws at openjdk.org> wrote:

>> Hi Andrew,
>> 
>> The current indexOf char intrinsics for StringUTF16 and the new one here for StringLatin1 both use the AVX2 – i.e. 256
>> bit instructions, these are also affected by the frequency scaling which affects the AVX-512 instructions you pointed
>> out. Of course in a world where all the work taking place is AVX instructions this wouldn’t be an issue but in mixed
>> mode execution this is a problem.  However, the compiler does have knowledge of the capability of the CPU upon which
>> it’s optimizing code for and is able to decide whether to use AVX instructions if they are supported by the CPU AND if
>> it wouldn’t be detrimental for performance. In fact, there is a flag which one can use to interact with
>> this: -XX:UseAVX=version.  This of course made testing this patch an interesting experience as the AVX2 instructions
>> were not enabled on the Xeon processors which I had access to at AWS, but in the end I was able to use an i7 on my
>> corporate macbook to validate the code.  From: mlbridge[bot] <notifications at github.com> Sent: 11 September 2020 17:01
>> To: openjdk/jdk <jdk at noreply.github.com> Cc: Tatton, Jason <jptatton at amazon.com>; Mention <mention at noreply.github.com>
>> Subject: Re: [openjdk/jdk] 8173585: Intrinsify StringLatin1.indexOf(char) (#71)
>> 
>> 
>> Mailing list message from Andrew Haley<mailto:aph at redhat.com> on hotspot-dev<mailto:hotspot-dev at openjdk.java.net>:
>> 
>> On 11/09/2020 11:23, Jason Tatton wrote:
>> 
>> For the x86 implementation there may be two further improvements we
>> can make in order to improve performance of both the StringUTF16 and
>> StringLatin1 indexOf(char) intrinsics:
>> 
>> 1. Make use of AVX-512 instructions.
>> 
>> Is this really a good idea?
>> 
>> When the processor detects Intel AVX instructions, additional
>> voltage is applied to the core. With the additional voltage applied,
>> the processor could run hotter, requiring the operating frequency to
>> be reduced to maintain operations within the TDP limits. The higher
>> voltage is maintained for 1 millisecond after the last Intel AVX
>> instruction completes, and then the voltage returns to the nominal
>> TDP voltage level.
>> 
>> https://computing.llnl.gov/tutorials/linux_clusters/intelAVXperformanceWhitePaper.pdf
>> 
>> So, if StringLatin1.indexOf(char) executes enough to make a difference
>> to any real-world program, the effect may well be to slow down the clock
>> for all of the code that does not use AVX.
>> 
>> 2. For ?short? Strings (see below), I think it may be possible to modify the existing algorithm to still use SSE SIMD
>> instructions instead of a loop.
>> 
>> --
>> Andrew Haley (he/him)
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://www.redhat.com>
>> https://keybase.io/andrewhaley
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>> 
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub<https://github.com/openjdk/jdk/pull/71#issuecomment-691179797>, or
>> unsubscribe<https://github.com/notifications/unsubscribe-auth/AQ44AL33DFEQ36TCHSTZKCLSFJCUJANCNFSM4Q74AKCQ>.
>
> Hi everyone,
> 
> This patch is just missing a couple of reviewers... Please can someone step forward?
> 
> I think it's a fairly straightforward change.
> 
> -Jason

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.

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

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