On Fri, 18 Sep 2020 11:04:34 GMT, Jason Tatton <github.com+70893615+jasontatton-...@openjdk.org> wrote:
>> 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. 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/71
