Fix mmap ReadOnlyMemory copy and add release notes - #20271
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
T-Gro
left a comment
There was a problem hiding this comment.
Nice, focused fix — thanks for chasing this one down and writing such a clear issue + PR description. 🙌 I read through it, and the diagnosis in #20263 is spot on: MemoryMappedFile.CreateViewStream returns a SafeBuffer-backed MemoryMappedFileViewStream, and UnmanagedMemoryStream.PositionPointer throws NotSupportedException on that stream type. So the old zero-copy path was fundamentally unsound for this stream, not just slow.
What I like about the fix
- Correct and minimal. Dropping
PositionPointerand going throughStream.Writeis exactly the supported mechanism the runtime expects for aSafeBuffer-backed view. The stream starts at position 0 and the view length equalsbytes.Length, so a singleWritefills the mapping exactly — and the position auto-advances, so the old manualstream.Position <- stream.Position + lengthis no longer needed. - Keeps the fast path.
MemoryMarshal.TryGetArraymeans the common array-backedReadOnlyMemory<byte>still copies with no extra managed allocation (Stream.Write(array, offset, count)is a directmemcpyinto theSafeBuffer). TheToArray()fallback only allocates in the genuinely rare non-array-backed case (e.g. aMemoryManager<byte>-backed memory), which is the case that was crashing before — so behaviorally it's strictly better everywhere. !!segment.Arrayis right. WhenTryGetArrayreturnstrue,Arrayis non-null, so the null-forgiving operator is justified and it matches the existing nullness style already used elsewhere inFileSystem.fs.- Portable across both TFMs. Using the array-based
Stream.Writeoverload (rather thanStream.Write(ReadOnlySpan<byte>)) keeps this compiling onnetstandard2.0as well asnet11.0.
Verification
I built FSharp.Compiler.Service locally in Debug — clean, 0 warnings / 0 errors for both netstandard2.0 and net11.0. The change compiles and the nullness annotations check out.
Minor, non-blocking nits
- Release-note placement. The changed code lives in
src/Compiler/Utilities/FileSystem.fs, i.e. FSharp.Compiler.Service. It might fit more naturally underdocs/release-notes/.FSharp.Compiler.Service/11.0.100.md's### Fixedsection rather than.VisualStudio/18.vNext.md— even though the symptom was observed while debugging in VS, the fix itself is in the compiler/service layer. Totally a judgment call, feel free to leave it. - Optional regression test. Since the failing path was specifically a non-array-backed
ReadOnlyMemory<byte>reachingTryFromMemory, a tiny test that drives that branch (e.g. aMemoryManager<byte>-backedReadOnlyMemory) would lock in the fix. Not required for such a targeted change, but it would guard against a future refactor reintroducing aPositionPointer-style shortcut.
Overall this is a clean, well-reasoned fix. LGTM. 👍
b05b9fd to
397d44c
Compare
397d44c to
507fb66
Compare
Summary
Fixes #20263 by avoiding the unsupported
PositionPointeraccess in the memory-mapped-file optimization path when copyingReadOnlyMemory<byte>intoMemoryMappedFileViewStream.The fix preserves the optimization while falling back to a safe
Stream.Writepath when the underlying memory is not a plain array-backedReadOnlyMemory.Changes
PositionPointer/Spancopy logic insrc/Compiler/Utilities/FileSystem.fswith a safe copy strategy.### Fixedsection for issueNotSupportedExceptionin memory-mapped-file optimization when copyingReadOnlyMemoryintoMemoryMappedFileViewStream#20263.