[SRM] Refactor reading from streams. (#111323)

Author: teo-tsirpanis

src/libraries/System.Reflection.Metadata/src/System.Reflection.Metadata.csproj

@@ -104,7 +104,6 @@ The System.Reflection.Metadata library is built-in as part of the shared framewo
     <Compile Include="System\Reflection\Internal\MemoryBlocks\MemoryBlockProvider.cs" />
     <Compile Include="System\Reflection\Internal\MemoryBlocks\MemoryMappedFileBlock.cs" />
     <Compile Include="System\Reflection\Internal\MemoryBlocks\NativeHeapMemoryBlock.cs" />
-    <Compile Include="System\Reflection\Internal\MemoryBlocks\StreamConstraints.cs" />
     <Compile Include="System\Reflection\Internal\MemoryBlocks\StreamMemoryBlockProvider.cs" />
     <Compile Include="System\Reflection\Internal\Utilities\BitArithmetic.cs" />
     <Compile Include="System\Reflection\Internal\Utilities\StringUtils.cs" />
@@ -113,7 +112,6 @@ The System.Reflection.Metadata library is built-in as part of the shared framewo
     <Compile Include="System\Reflection\Internal\Utilities\StreamExtensions.netcoreapp.cs" Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'" />
     <Compile Include="System\Reflection\Internal\Utilities\StreamExtensions.netstandard2.0.cs" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'" />
     <Compile Include="System\Reflection\Internal\Utilities\Hash.cs" />
-    <Compile Include="System\Reflection\Internal\Utilities\ImmutableMemoryStream.cs" />
     <Compile Include="System\Reflection\Internal\Utilities\MemoryBlock.cs" />
     <Compile Include="System\Reflection\Internal\Utilities\PooledStringBuilder.cs" />
     <Compile Include="System\Reflection\Internal\Utilities\ObjectPool`1.cs" />

src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/AbstractMemoryBlock.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections.Immutable;
+using System.IO;
 using System.Reflection.Metadata;
 
 namespace System.Reflection.Internal
@@ -23,6 +24,11 @@ internal abstract class AbstractMemoryBlock : IDisposable
 
         public unsafe BlobReader GetReader() => new BlobReader(Pointer, Size);
 
+        /// <summary>
+        /// Creates a new stream wrapping the block's memory.
+        /// </summary>
+        public unsafe Stream GetStream() => new UnmanagedMemoryStream(Pointer, Size);
+
         /// <summary>
         /// Returns the content of the entire memory block.
         /// </summary>

src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/ByteArrayMemoryProvider.cs

@@ -34,12 +34,6 @@ protected override AbstractMemoryBlock GetMemoryBlockImpl(int start, int size)
             return new ByteArrayMemoryBlock(this, start, size);
         }
 
-        public override Stream GetStream(out StreamConstraints constraints)
-        {
-            constraints = new StreamConstraints(null, 0, Size);
-            return new ImmutableMemoryStream(_array);
-        }
-
         internal unsafe byte* Pointer
         {
             get

src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/ExternalMemoryBlockProvider.cs

@@ -33,12 +33,6 @@ protected override AbstractMemoryBlock GetMemoryBlockImpl(int start, int size)
             return new ExternalMemoryBlock(this, _memory + start, size);
         }
 
-        public override Stream GetStream(out StreamConstraints constraints)
-        {
-            constraints = new StreamConstraints(null, 0, _size);
-            return new UnmanagedMemoryStream(_memory, _size);
-        }
-
         protected override void Dispose(bool disposing)
         {
             Debug.Assert(disposing);

src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/MemoryBlockProvider.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Reflection.PortableExecutable;
 
@@ -39,12 +40,21 @@ public AbstractMemoryBlock GetMemoryBlock(int start, int size)
         protected abstract AbstractMemoryBlock GetMemoryBlockImpl(int start, int size);
 
         /// <summary>
-        /// Gets a seekable and readable <see cref="Stream"/> that can be used to read all data.
-        /// The operations on the stream has to be done under a lock of <see cref="StreamConstraints.GuardOpt"/> if non-null.
-        /// The image starts at <see cref="StreamConstraints.ImageStart"/> and has size <see cref="StreamConstraints.ImageSize"/>.
-        /// It is the caller's responsibility not to read outside those bounds.
+        /// Gets the <see cref="Stream"/> backing the <see cref="MemoryBlockProvider"/>, if there is one.
         /// </summary>
-        public abstract Stream GetStream(out StreamConstraints constraints);
+        /// <remarks>
+        /// It is the caller's responsibility to use <paramref name="stream"/> only
+        /// while locking on <paramref name="streamGuard"/>, and not read outside the
+        /// bounds defined by <paramref name="imageStart"/> and <paramref name="imageSize"/>.
+        /// </remarks>
+        public virtual bool TryGetUnderlyingStream([NotNullWhen(true)] out Stream? stream, out long imageStart, out int imageSize, [NotNullWhen(true)] out object? streamGuard)
+        {
+            stream = null;
+            imageStart = 0;
+            imageSize = 0;
+            streamGuard = null;
+            return false;
+        }
 
         /// <summary>
         /// The size of the data.

src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamConstraints.cs

@@ -28,7 +28,7 @@ internal sealed class StreamMemoryBlockProvider : MemoryBlockProvider
         private readonly object _streamGuard;
 
         private readonly bool _leaveOpen;
-        private bool _useMemoryMap;
+        private readonly bool _useMemoryMap;
 
         private readonly long _imageStart;
         private readonly int _imageSize;
@@ -73,13 +73,7 @@ internal static unsafe NativeHeapMemoryBlock ReadMemoryBlockNoLock(Stream stream
             try
             {
                 stream.Seek(start, SeekOrigin.Begin);
-
-                int bytesRead = 0;
-
-                if ((bytesRead = stream.Read(block.Pointer, size)) != size)
-                {
-                    stream.CopyTo(block.Pointer + bytesRead, size - bytesRead);
-                }
+                stream.ReadExactly(block.Pointer, size);
 
                 fault = false;
             }
@@ -94,19 +88,23 @@ internal static unsafe NativeHeapMemoryBlock ReadMemoryBlockNoLock(Stream stream
             return block;
         }
 
+        public override bool TryGetUnderlyingStream([NotNullWhen(true)] out Stream? stream, out long imageStart, out int imageSize, [NotNullWhen(true)] out object? streamGuard)
+        {
+            stream = _stream;
+            imageStart = _imageStart;
+            imageSize = _imageSize;
+            streamGuard = _streamGuard;
+            return true;
+        }
+
         /// <exception cref="IOException">Error while reading from the stream.</exception>
         protected override AbstractMemoryBlock GetMemoryBlockImpl(int start, int size)
         {
             long absoluteStart = _imageStart + start;
 
             if (_useMemoryMap && size > MemoryMapThreshold)
             {
-                if (TryCreateMemoryMappedFileBlock(absoluteStart, size, out MemoryMappedFileBlock? block))
-                {
-                    return block;
-                }
-
-                _useMemoryMap = false;
+                return CreateMemoryMappedFileBlock(absoluteStart, size);
             }
 
             lock (_streamGuard)
@@ -115,26 +113,18 @@ protected override AbstractMemoryBlock GetMemoryBlockImpl(int start, int size)
             }
         }
 
-        public override Stream GetStream(out StreamConstraints constraints)
-        {
-            constraints = new StreamConstraints(_streamGuard, _imageStart, _imageSize);
-            return _stream;
-        }
-
         /// <exception cref="IOException">IO error while mapping memory or not enough memory to create the mapping.</exception>
-        private unsafe bool TryCreateMemoryMappedFileBlock(long start, int size, [NotNullWhen(true)] out MemoryMappedFileBlock? block)
+        private unsafe MemoryMappedFileBlock CreateMemoryMappedFileBlock(long start, int size)
         {
             if (_lazyMemoryMap == null)
             {
-                // leave the underlying stream open. It will be closed by the Dispose method.
-                MemoryMappedFile newMemoryMap;
-
                 // CreateMemoryMap might modify the stream (calls FileStream.Flush)
                 lock (_streamGuard)
                 {
                     try
                     {
-                        newMemoryMap =
+                        // leave the underlying stream open. It will be closed by the Dispose method.
+                        _lazyMemoryMap ??=
                             MemoryMappedFile.CreateFromFile(
                                 fileStream: (FileStream)_stream,
                                 mapName: null,
@@ -148,17 +138,6 @@ private unsafe bool TryCreateMemoryMappedFileBlock(long start, int size, [NotNul
                         throw new IOException(e.Message, e);
                     }
                 }
-
-                if (newMemoryMap == null)
-                {
-                    block = null;
-                    return false;
-                }
-
-                if (Interlocked.CompareExchange(ref _lazyMemoryMap, newMemoryMap, null) != null)
-                {
-                    newMemoryMap.Dispose();
-                }
             }
 
             MemoryMappedViewAccessor accessor;
@@ -168,14 +147,7 @@ private unsafe bool TryCreateMemoryMappedFileBlock(long start, int size, [NotNul
                 accessor = _lazyMemoryMap.CreateViewAccessor(start, size, MemoryMappedFileAccess.Read);
             }
 
-            if (accessor == null)
-            {
-                block = null;
-                return false;
-            }
-
-            block = new MemoryMappedFileBlock(accessor, accessor.SafeMemoryMappedViewHandle, accessor.PointerOffset, size);
-            return true;
+            return new MemoryMappedFileBlock(accessor, accessor.SafeMemoryMappedViewHandle, accessor.PointerOffset, size);
         }
     }
 }

src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamMemoryBlockProvider.cs

@@ -9,36 +9,6 @@ namespace System.Reflection.Internal
 {
     internal static partial class StreamExtensions
     {
-        // From System.IO.Stream.CopyTo:
-        // We pick a value that is the largest multiple of 4096 that is still smaller than the large object heap threshold (85K).
-        // The CopyTo/CopyToAsync buffer is short-lived and is likely to be collected at Gen0, and it offers a significant
-        // improvement in Copy performance.
-        internal const int StreamCopyBufferSize = 81920;
-
-        /// <summary>
-        /// Copies specified amount of data from given stream to a target memory pointer.
-        /// </summary>
-        /// <exception cref="IOException">unexpected stream end.</exception>
-        internal static unsafe void CopyTo(this Stream source, byte* destination, int size)
-        {
-            byte[] buffer = new byte[Math.Min(StreamCopyBufferSize, size)];
-            while (size > 0)
-            {
-                int readSize = Math.Min(size, buffer.Length);
-                int bytesRead = source.Read(buffer, 0, readSize);
-
-                if (bytesRead <= 0 || bytesRead > readSize)
-                {
-                    throw new IOException(SR.UnexpectedStreamEnd);
-                }
-
-                Marshal.Copy(buffer, 0, (IntPtr)destination, bytesRead);
-
-                destination += bytesRead;
-                size -= bytesRead;
-            }
-        }
-
         /// <summary>
         /// Attempts to read all of the requested bytes from the stream into the buffer
         /// </summary>