Initial rewrite of MMapDirectory for JDK-16 preview (incubating) Panama APIs (>= JDK-16-ea-b32) #2176
Conversation
+1 Things work like they do today because auto-vectorization only works on on-heap arrays, and this readLELongs method was the fastest way to copy data from a directory to a long[]. @msokolov and I were just discussing a few days ago how this vector API might help drop the copy entirely in the future, which would be great. |
|
Very exciting! Thank you for leading the way here, Uwe.
I ran into this just yesterday as I was playing around with aligning vectors in their index files to see if any perf bump could be gained (the header seems to be 50 bytes usually, so some padding is needed). And then when I asserted alignment, realized that CFS was messing with it. |
In fact for CFS file we don't even need to change the file format / version number. We just have to make sure that CFS writer starts new inner files aligned to 8 bytes. That should be easy to implement. |
|
@dweiss I need your help here. There is one thing that drives me crazy: with the changes (actually adding This seems to have to do with some automatism by Gradle or by JUnit once it detects the "Java module system", and I have no idea how to turn it off! What I am completely wondering about: why does it load those classes at all? I was trying to find the good old "Ant fileset" that has this Unless this is fixed, I can't run all tests easily. I only ran them per module and ignored the tons of stack traces. PLEASE HELP! |
…d from ANT build)
|
@dweiss I found a workaround by adding the include/excludes as we use in Ant build. Why were those removed? Should I maybe reopen a new issue to add them back in master? I have the feeling thois comes from the fact that class files compiled with JDK16 and using the new class file format can't be "analyzed" by gradle, so it assumes "no idea, let's run it because its a class file". The automatisms don't seem to work then. With the include/exclude patterns everything went back to normal. Should we also add those lines to |
|
FYI, after I was able to run tests in a more convenient way, I figured out that some tests fail from time to time because of using I forgot to mention this in my description above: MMapDirectory no longer works with custom |
…s occur! Remove useless slicing if aligned.
|
I fixed the rmeinaing TODOs regarding a safe close of all segments, when exceptions on As MemorySegment does not implement |
…ning "buffer" to "segment"; also make the segments array final (curSegment == null when closed)
|
After some cleanup, I also added a workaround for https://bugs.openjdk.java.net/browse/JDK-8259028: In MMapDirectory we try to unwrap the path using reflection. This is for sure a hackydihickhack workaround, but works until this is fixed -- and now all tests pass for me! :-) |
|
Hi Uwe - just trying to get this patch working here; when I try to compile ( |
|
Ah, OK once I set |
|
Hi read the instructions above, at "be aware". Gradle is not compatible to JDK 16 at all. So Gradle needs to run with JDK 11. You can pass the JDK 16 directory using an environment variable or through sysprop. Gradlew shows all options when you run it without any options and read instructions about alternative jvms. |
|
See this, Mike: |
lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java
Outdated
Show resolved
Hide resolved
uschindler
force-pushed
the
draft/jdk-foreign-mmap
branch
from
570a31b
to
c56851c
Compare
|
This would also explain slowdowns on short posting-list iterations, as readLELongs() with smaller number of longs is not working well. My plan would be to do an if statement at beginning of readBytes() or readLELongs() like: if (length < SOME_LIMIT) {
for (int i = offset; i< offset + length; i++) {
bytes[i] = readByte();
}
} else {
... current code with targetSlice ...
} |
|
Uwe, I didn't think that IndexInput would expose its internal ByteBuffers easily? But you are right about the double-copying - that is why I opened https://issues.apache.org/jira/browse/LUCENE-9652. BTW I did test that change with and without this one and saw a similar slowdown there. Possibly a loop with a single float at a time would be better? It's highly counterintutitive to me, but when I get a moment, I will try it. |
lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java
Show resolved
Hide resolved
uschindler
force-pushed
the
draft/jdk-foreign-mmap
branch
from
8988de4
to
50d9300
Compare
…eException: Cannot close while another thread is accessing the segment"
…ng objects to extend their functionality (like asserting in tests)
|
FYI, I added some interface in oal.util.Unwrapable that can be implemented by our mocking layers in test-framework. The new interface allows us to unwrap the Path without knowing about test-framework internals. IMHO, we should use this also for other mocking layers, so we can easy unwrap them with some consistent API. The remaining 2 issues in MappedByteBuffer.mapFile() were merged today and as soon as those are part of another preview build of JDK 16, we can remove the mapFileBugfix() method. |
…eap segments don't need this)
uschindler
force-pushed
the
draft/jdk-foreign-mmap
branch
from
5336b6b
to
60200e8
Compare
|
Hi @msokolov: If you have so time, you may check performance again. I rewrote the getBytes() and getLEXxxx() methods a bit. |
|
@uschindler I pulled latest from this branch (ba61072) and re-ran (comparing to updated master (6711eb7) and see similar results: |
|
I removed the hacks for the bugs in JDK. The minimum requirement to test this draft implementation is JDK-16-ea-b32. Thanks @msokolov for the performance tests. I was hoping that it gets a bit better, but we may need to figure out where the speed differences are coming from. I don't think results will change in the new preview build, I just removed the hacks. |
uschindler
changed the title
… can correctly throw AlreadyClosedEx; TODO: add a test
|
I will close this and would like to move discussion over to apache/lucene#173 |
This is just a draft PR for a first insight on memory mapping improvements in JDK 16+.
Some background information: Starting with JDK-14, there is a new incubating module "jdk.incubator.foreign" that has a new, not yet stable API for accessing off-heap memory (and later it will also support calling functions using classical MethodHandles that are located in libraries like .so or .dll files). This incubator module has several versions:
This module more or less overcomes several problems:
sun.misc.Unsafeand forcefully unmap segments, but any IndexInput accessing the file from another thread will crush the JVM with SIGSEGV or SIGBUS. We learned to live with that and we happily apply the unsafe unmapping, but that's the main issue.@uschindler had many discussions with the team at OpenJDK and finally with the third incubator, we have an API that works with Lucene. It was very fruitful discussions (thanks to @mcimadamore !)
With the third incubator we are now finally able to do some tests (especially performance). As this is an incubating module, this PR first changes a bit the build system:
-Werrorfor:lucene:core:lucene:coreand enable it for all test builds. This is important, as you have to pass--add-modules jdk.incubator.foreignalso at runtime!The code basically just modifies
MMapDirectoryto use LONG instead of INT for the chunk size parameter. In addition it addsMemorySegmentIndexInputthat is a copy of ourByteBufferIndexInput(still there, but unused), but using MemorySegment instead of ByteBuffer behind the scenes. It works in exactly the same way, just the try/catch blocks for supporting EOFException or moving to another segment were rewritten.The openInput code uses
MemorySegment.mapFile()to get a memory mapping. This method is unfortunately a bit buggy in JDK-16-ea-b30, so I added some workarounds. See JDK issues: https://bugs.openjdk.java.net/browse/JDK-8259027, https://bugs.openjdk.java.net/browse/JDK-8259028, https://bugs.openjdk.java.net/browse/JDK-8259032, https://bugs.openjdk.java.net/browse/JDK-8259034. The bugs with alignment and zero byte mmaps are fixed in b32, this PR was adapted (hacks removed).It passes all tests and it looks like you can use it to read indexes. The default chunk size is now 16 GiB (but you can raise or lower it as you like; tests are doing this). Of course you can set it to Long.MAX_VALUE, in that case every index file is always mapped to one big memory mapping. My testing with Windows 10 have shown, that this is not a good idea!!!. Huge mappings fragment address space over time and as we can only use like 43 or 46 bits (depending on OS), the fragmentation will at some point kill you. So 16 GiB looks like a good compromise: Most files will be smaller than 6 GiB anyways (unless you optimize your index to one huge segment). So for most Lucene installations, the number of segments will equal the number of open files, so Elasticsearch huge user consumers will be very happy. The sysctl max_map_count may not need to be touched anymore.
In addition, this implements
readLELongsin a better way than @jpountz did (no caching or arbitrary objects). Nevertheless, as the new MemorySegment API relies on final, unmodifiable classes and coping memory from a MemorySegment to a on-heap Java array, it requires us to wrap all those arrays using a MemorySegment each time (e.g. inreadBytes()orreadLELongs), there may be some overhead du to short living object allocations (those are NOT reuseable!!!). In short: In future we should throw away on coping/loading our stuff to heap and maybe throw away IndexInput completely and base our code fully on random access. The new foreign-vector APIs will in future also be written with MemorySegment in its focus. So you can allocate a vector view on a MemorySegment and let the vectorizer fully work outside java heap inside our mmapped files! :-)It would be good if you could checkout this branch and try it in production.
But be aware:
JAVA_HOMEto it)RUNTIME_JAVA_HOMEto it)--add-modules jdk.incubator.foreignto the command line of your Java program/Solr server/Elasticsearch serverIt would be good to get some benchmarks, especially by @rmuir or @mikemccand. Take your time and enjoy the complexity of setting this up! ;-)
My plan is the following:
In addition there are some comments in the code talking about safety (e.g., we needIn addition, by default all VarHandles are aligned. By default it refuses to read a LONG from an address which is not a multiple of 8. I had to disable this feature, as all our index files are heavily unaliged. We should in meantime not only convert our files to little endian, but also make all non-compressed types (likeIOUtils.close()takingAutoCloseableinstead of justCloseable, so we can also enfoce that all memory segments are closed after usage.long[]arrays or non-encoded integers be aligned to the correct boundaries in files). The most horrible thing I have seen is that our CFS file format starts the "inner" files totally unaligned. We should fix the CFSWriter to start new files always at multiples of 8 bytes. I will open an issue about this.