If this is MR-jar code then it'd be cleaner to create separate sourcesets for different jdk levels required and then apply these compiler options to each sourceset. This makes it possible to work with multiple "levels" of compatibility on the same source data.

You could also (conditionally) disable compilation of these extra sourcesets if the compiler version is lower than the sourceset's required version... this would enable people to work with the code without installing all the (alpha) JDKs.

I can take care of this if needed - let me know.

The problem is currently: it is incubating API, so it will break with JDK17 again.

We have to wait that the incubating module get's into java.base / java.whatever, so it can be used and API is stable. The problem with MR-JARs is the following:

• it does not handle module system, so you would get ClassNotFound if you dont start with correct command line flag
• You can't say: "use this only is Java version exactly 16"

Like with my own Pull Request for MMAP: We should test this, e.g. on Jenkins, but not add to productive code.

I feel the same for MMapDirectory v2: "it should really go into Lucene Core", with some trick, but at the moment I have to wait. Netty has something similar at moment, you have to download a separate JAR and add it to your classpath.

So one way to implement this: Add a separate lucene module that's optional and use some "plug in" mechanism (e.g., SPI). People who want to use it must use the exact correct version for their JDK version and must add some command line flag to use it (enable it in their JDK).

For MMapDirectory my plan would be to have a separate lucene-mmap-foreign-jdk16.jar that compiled against JDK 16 that provides another directory. Code that wants to use it knows:

• you have to add the JAR file
• you have to hardcode new JDK16MMapDirectory() instead of FSDircetory.open()
• add command line flags

Is it really impossible though? Wit MR-JAR could we define a class version for 16+, but define slow version for 17+? Effectively, we define a version of the code for only exactly version 16?

Furthermore, couldn't we check with static initializer and if the incubator module isn't available, link methodhandle to a slow version?

I think: for this use case, we should not use MR-JARs at all. Just use a MethodHandle. It's just a single method and a few classes. Maybe you can construct this using a single MethodHandle using those combiners (MethodHandles.adapt.... and MH.guard, and all other methods).... Another MethodHandle would go to our slow version.

I'll just repeat what I think I'm hearing since I'm slow ... It sounds like a way forward is something like this?

1. Use reflection to search for this new implementation of static float SuperDuperVectors.dotProduct(float[], float[]) and/or fall back to the old way?
2. Provide a {{vector-api.jar}} with SuperDuperVectors in it (or some such) just to wrap this object, and maybe other Vector stuff like Euclidean norm.
3. Get the build sorted out so that the plugin can be built conditionally, depending on JDK used.

Then users can incorporate that jar and provide the proper JDK command line flag to include the incubating vector api support, and that's it?

Not reflection, but a combined MethodHandle.

Does it fails with JDK16, even if it's not used?

yes, unfortunately it does:

$ ./gradlew -Pruntime.java.home=/home/rmuir/Downloads/jdk-16 compileJava
...
> Task :lucene:core:compileJava FAILED
Exception in thread "main" java.lang.IllegalAccessError: class com.google.errorprone.ErrorProneJavacPlugin (in unnamed module @0x31610302) cannot access class com.sun.tools.javac.api.BasicJavacTask (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.api to unnamed module @0x31610302
        at com.google.errorprone.ErrorProneJavacPlugin.init(ErrorProneJavacPlugin.java:38)
        at jdk.compiler/com.sun.tools.javac.api.BasicJavacTask.initPlugin(BasicJavacTask.java:255)
        at jdk.compiler/com.sun.tools.javac.api.BasicJavacTask.initPlugins(BasicJavacTask.java:229)
        at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:292)
        at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:176)
        at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:64)
        at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:50)

I'll handle this separately.

I committed a different variant of jdk16 workaround and ran check, worked for me.

I think the problem is that the errorprone plugin has some META-INF/service, so as soon as it appears on classpath it is hooked into javac.

So you latest fix seem to work around it (keep the dependency, but remove it from config, so it is not added to classpath). I did not fully understand you patch, but this looks fine.

very elegant! We had played around with this API last summer and failed to achieve gains. Maybe it has incubated! Or maybe we fumbled the attempt; anyway this is awesome gains for a very small amount of code which is right at the hot spot for vector search. I'll leave some thoughts about what strategy to pursue for making this available on the JIRA, but as for the code, it's great

@msokolov here's a few notes of troubles i ran into in case it helps for the next time:

• start with "full-performance" example in the JEP/javadocs and then iterate from there.
• don't use "single masked vector op" at the end. don't use masked ops at all :)
• don't put kernel "last" like current code in master. it must be "first" (alignment restrictions i think).
• loop structure is important, otherwise whole loop gets slow (bounds checks I think). See the tricks i had to do with upperBound: that's the only way i was able to get a fast loop.

I may revisit the existing scalar code in master and see if things can be improved there, too.

oh man, this is pretty funny, evil whatever. It's nice that it's self contained and doesn't require any build shenanigans. I guess it might be difficult to modify the source code though :) I'll run a test in a vector benchmark to see what happens there

So I ran my usual HNSW benchmark using KnnGraphTester, which is 1M 256-dim vectors. This was run on my 2-core i7 laptop . Indexing performance improved from 412 sec to 292 sec, and search times improved somewhat less (latency column in table below). These benchmarks are kind of noisy, but they are predictive of what performance we'd expect in an application. Probably we aren't able to pump vectors through the L2 cache fast enough to keep up with the CPU at this point. We might be able to restructure the search code to try to do some pipelining?

JDK16 --add-modules jdk.incubator.vector

recall latency nDoc fanout maxConn beamWidth visited index ms
0.881 0.49 1000000 10 20 50 1154 291684
0.897 0.60 1000000 25 20 50 1262 0
0.913 0.63 1000000 50 20 50 1444 0
0.934 0.92 1000000 100 20 50 1791 0

JDK-11

0.884 0.63 1000000 10 20 50 1143 412111
0.888 0.66 1000000 25 20 50 1248 0
0.913 0.78 1000000 50 20 50 1434 0
0.929 1.00 1000000 100 20 50 1783 0

Thanks for testing (high level) @msokolov ! I think you are correct that is expected, it would not translate exactly to 5x faster indexing or whatever, as it isn't the only thing that is happening.

From my inspection, there are a lot of other hotspots there, too.

But yeah, it is true that maybe we can start working those other hotspots off as well. For example, IMO it is silly with mmap directory for us to be decoding byte[] slowly into a float[] (readLEFloats or whatever). Vector API can use byte[] or even ByteBuffer directly (I assume any conversions are vectorized too, have not experimented with that).

refs:
https://download.java.net/java/early_access/jdk16/docs/api/jdk.incubator.vector/jdk/incubator/vector/FloatVector.html#fromByteArray(jdk.incubator.vector.VectorSpecies,byte%5B%5D,int,java.nio.ByteOrder)

https://download.java.net/java/early_access/jdk16/docs/api/jdk.incubator.vector/jdk/incubator/vector/FloatVector.html#fromByteBuffer(jdk.incubator.vector.VectorSpecies,java.nio.ByteBuffer,int,java.nio.ByteOrder)

I think we should have the source code of this class file available for regeneration. So anybody can compile with Java 11, but regeneration is only possible with exact Java 16 version.
As it currently looks like I would not be sure if we can release this.

If we don't add the original source code as a java file for regeneration, we should add source code here (as comment).

But yeah, it is true that maybe we can start working those other hotspots off as well. For example, IMO it is silly with mmap directory for us to be decoding byte[] slowly into a float[] (readLEFloats or whatever). Vector API can use byte[] or even ByteBuffer directly (I assume any conversions are vectorized too, have not experimented with that).

It gets even worse with MMapDirectory version 2 for Java 16. So IMHO, once we are really on JDK 17 minimum, we should change the method signatures of IndexInpout and replace our void readLEFloats(float[]) by FloatVector readFloatVector(), on MMapDirectory this can use a ByteBuffer oder MemorySegmrnt directly on the mmapped contents (if platform endianess is LE). This would spare millions of native->heap arraycopy actions for nonsense.

start with changing it to use byte[] instead of float[]. For the case of vector 256 this replaces 64 calls each to bytebuffer position, readInt, bswap, etc etc with a single readBytes() call. And it is easiest next step as there will be no alignment issues.

going from bytebuffer is harder.

This doesn't require additional permissions (custom class loader definition)?

It's safe, because we catch Throwable. To be safe, maybe add AccessController around it. But the necessity for this requires more investigation, I am sure Robert checked this. :-)

I think, we don't need extra permissions, as we only work in same java package. The code here is copypasted from Expressions module.

it requires createClassLoader which is documented as evil. sorry, i didnt try to make this PR "clean" or refine it.

I tried to work on solutions, and got very frustrated. this little function should be 5 lines of code in almost any programming language. But a combination of things here add up to make the java programming language TERRIBLE. I quickly through this together as an act of retaliation.

So I need a break from it as I am too frustrated. If you wish to take over the PR, please feel free!

I'd add another catch with Error.

I think we should have the source code of this class file available for regeneration. So anybody can compile with Java 11, but regeneration is only possible with exact Java 16 version.
As it currently looks like I would not be sure if we can release this.

If we don't add the original source code as a java file for regeneration, we should add source code here (as comment).

this signature is strange as it takes the bytes, but not the class name. Either take class name, too, or alternatively decode pass the base64 directly here: Base64.getMimeDecoder().decode(SIMD_BASE64), so method gets parameterless.

Ideally (as we are on Java 16), we could have used the new anonymous classes. But this code is also not visible, damn!

It's safe, because we catch Throwable. To be safe, maybe add AccessController around it. But the necessity for this requires more investigation, I am sure Robert checked this. :-)

I think, we don't need extra permissions, as we only work in same java package. The code here is copypasted from Expressions module.

Move MethodHandles.lookup() to top of static block, as we need it multiple times.