[PR] Redesign MMapDirectory to fix errors and improve performance, #1013, #1151 [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin opened a new pull request, #1361:
URL: https://github.com/apache/lucenenet/pull/1361

   - [X] You've read the [Contributor Guide](https://github.com/apache/lucenenet/blob/master/CONTRIBUTING.md) and [Code of Conduct](https://www.apache.org/foundation/policies/conduct.html).
   - [X] You've included unit or integration tests for your change, where applicable.
   - [X] You've included inline docs for your change, where applicable.
   - [X] There's an open issue for the PR that you are making. If you'd like to propose a change, please [open an issue](https://github.com/apache/lucenenet/issues/new/choose) to discuss the change or find an existing issue.
   
   Redesign MMapDirectory to fix errors and improve performance
   
   Fixes #1013 
   Fixes #1151
   
   ## Description
   
   This is a redo of PR #1267 which was growing quite long with a lot of already-addressed comments, plus this factors in a newer, simplified approach. It rebuilds `MMapDirectory`'s read engine on `MemoryMappedViewAccessor` raw pointers and addresses two long-standing issues together:
   
   - **#1013** - sporadic `AccessViolationException` (a hard process crash) during concurrent search with `SearcherManager`. Root cause: a chunk's memory-mapped view could be unmapped on one thread (e.g. when a reader is disposed as the searcher swaps readers) while another thread was mid-read through a raw pointer into that view.
   - **#1151** - `MMapDirectory` was many times slower than `SimpleFSDirectory` under concurrent load. Root cause: the previous design took a native reference (`SafeBuffer.AcquirePointer`) [on *every read*](https://github.com/dotnet/dotnet/blob/e9cde69d4ee92243ccd47d9dd0b2ffd5f9cf4c3c/src/runtime/src/libraries/System.Private.CoreLib/src/System/IO/UnmanagedMemoryAccessor.cs#L124), a contended atomic that does not scale across threads, and is made worse in the single-byte-read case.
   
   Both are fixed by the same design: read from a cached raw pointer with one bounds check per read (no per-read native access), take a native reference only once per chunk crossing, and use the view's `SafeHandle` refcount as a drain barrier so a view cannot be unmapped while any reader holds a reference.
   
   ## Design
   
   - **Cached-pointer reads.** Each file is mapped as a fixed-size array of chunks (`1 << chunkSizePower` bytes), matching upstream Java's `maxChunkSize` model. A read is `*(readBase + pos)` after a single `pos < currentEnd` check. The native reference is acquired per chunk-crossing, not per read, so it is off the hot path entirely.
   
   - **`SafeHandle` drain barrier.** `SafeMemoryMappedViewHandle.AcquirePointer`/`ReleasePointer` per chunk-crossing is the native drain barrier: `accessor.Dispose()` defers the actual `UnmapViewOfFile` until every outstanding reference drains, and a crossing into a closed chunk fails fast with `AlreadyClosedException` instead of dereferencing freed memory. This replaces a hand-rolled lock-free state machine from earlier iterations.
   
   - **Cross-thread teardown.** A *same-thread* `Dispose` releases its chunk reference deterministically (the disposing thread is provably not mid-dereference). A *cross-thread* `Dispose` (the #1013 case: a reader torn down on another thread while a query is still reading) cannot release synchronously without risking an AVE, so it hands the reference to a small finalizable releaser that releases it once the input is unreachable (hence no reader can be in flight). This mirrors the JDK's own model: a Java `MappedByteBuffer` and its mapping "remain valid until the buffer itself is garbage-collected," and default (non-unmap-hack) `MMapDirectory` defers every unmap to GC. We defer to GC only this one residual case and release deterministically everywhere else: a good middle-ground between the non-unmap-hack and unmap-hack cases that each have undesirable outcomes (unmap-hack SIGSEGV's, non-unmap-hack defers to GC always).
   
   - **Reusable read engine.** The native read logic (cursor, fast read paths, chunk-crossing, teardown coordination) is extracted into `Support/UnsafeChunkIndexInput`, an abstract base whose only seam is a chunk source (`TryAcquireChunk`/`ReleaseChunk`/`ChunkLength`/`ChunkCount`). `MMapDirectory.MMapIndexInput` is now a thin subclass that supplies memory-mapped views. There is no upstream counterpart (Java keeps this in `ByteBufferIndexInput`), so it lives under `Support/` per convention.
   
   ## Benchmarks (net10.0, 16 cores, 64 MiB file)
   
   Single-threaded read throughput is on par with or better than `SimpleFSDirectory`, and concurrent reads scale near-linearly where the old design (and `SimpleFSDirectory`) collapse:
   
   | Scenario | MMap (new) | SimpleFS |
   |---|---|---|
   | Concurrent read x1 | 819 MiB/s | 585 MiB/s |
   | Concurrent read x4 | 3178 MiB/s | 912 MiB/s |
   | Concurrent read x8 | **6271 MiB/s** | 1065 MiB/s |
   
   `#1151` is decisively fixed: aggregate throughput grows with thread count instead of flat-lining.
   
   ## Testing
   
   - A managed-`byte[]`-backed test fixture (`TestUnsafeChunkIndexInput`) exercises the engine's concurrency logic deterministically (chunk-crossing reads, fail-fast on close, and the cross-thread strand interleaving) without real mmap, so failures surface as wrong values rather than host crashes.
   - `TestMultiMMap` adds real-mmap stress tests for the #1013 race (concurrent clone-read vs cross-thread dispose, slice-read vs slicer-dispose), plus deterministic leak-gate tests for the file-handle/view lifecycle. The race-stress tests are marked `[Nightly]`.
   
   ## Notes
   
   - A separate experiment confirmed that replacing the cross-thread finalizer with a synchronous lock is possible but costs 3-5x on the read hot path and erases the #1151 concurrency win (a locked MMap performs like `SimpleFSDirectory`). The finalizer is retained for that reason; it is strictly less GC reliance than upstream's default `MMapDirectory`, which defers all unmaps to GC.
   - Minor: a `ConcurrentDictionaryExtensions.TryRemove` polyfill for pre-.NET 5 targets, and a clarifying comment on `NIOFSDirectory`'s `FileShare` flags (matches Java's `FileChannel` default; no behavior change).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Redesign MMapDirectory to fix errors and improve performance, #1013, #1151 [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin closed pull request #1361: Redesign MMapDirectory to fix errors and improve performance, #1013, #1151
URL: https://github.com/apache/lucenenet/pull/1361


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Redesign MMapDirectory to fix errors and improve performance, #1013, #1151 [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin commented on PR #1361:
URL: https://github.com/apache/lucenenet/pull/1361#issuecomment-4743468871

   Update: I have run this branch on macOS and Windows repeatedly, as well as GitHub Actions and Azure DevOps, with zero failures. Ready for review.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Redesign MMapDirectory to fix errors and improve performance, #1013, #1151 [lucenenet]

["paulirwin (via GitHub)" <gi...@apache.org>]

paulirwin commented on PR #1361:
URL: https://github.com/apache/lucenenet/pull/1361#issuecomment-4770170754

   Marking as draft in favor of #1373


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org