From f6b854b17a9df17bc65c06e57d358640bc564ced Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Oct 2022 17:53:56 +0300 Subject: [PATCH 01/16] Use Unsafe.As in ImmutableByteArrayInterop. Code quality increases. --- .../Utilities/ImmutableByteArrayInterop.cs | 37 +++++-------------- 1 file changed, 9 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs index 7b213d337c2cec..7fb48d0d26feaa 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs @@ -2,7 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Immutable; -using System.Runtime.InteropServices; +using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; namespace System.Reflection.Internal { @@ -22,16 +23,10 @@ namespace System.Reflection.Internal /// /// This implementation is scoped to byte arrays as that is all that the metadata reader needs. /// - /// Also, since we don't have access to immutable collection internals, we use a trick involving - /// overlapping a with an array reference. While - /// unverifiable, it is valid. See ECMA-335, section II.10.7 Controlling instance layout: + /// Also, since we don't have access to immutable collection internals, we use + /// . /// - /// "It is possible to overlap fields in this way, though offsets occupied by an object reference - /// shall not overlap with offsets occupied by a built-in value type or a part of - /// another object reference. While one object reference can completely overlap another, this is - /// unverifiable." - /// - /// Furthermore, the fact that backed by a single byte array + /// The fact that is backed by a single byte array /// field is something inherent to the design of ImmutableArray in order to get its performance /// characteristics and therefore something we (Microsoft) are comfortable defining as a contract that /// can be depended upon as below. @@ -54,14 +49,12 @@ internal static unsafe class ImmutableByteArrayInterop /// whose underlying backing field is modified. /// /// An immutable array. - internal static ImmutableArray DangerousCreateFromUnderlyingArray(ref byte[]? array) + internal static ImmutableArray DangerousCreateFromUnderlyingArray([DisallowNull] ref byte[]? array) { - byte[] givenArray = array!; + byte[] givenArray = array; array = null; - ByteArrayUnion union = default; - union.UnderlyingArray = givenArray; - return union.ImmutableArray; + return Unsafe.As>(ref givenArray); } /// @@ -81,19 +74,7 @@ internal static ImmutableArray DangerousCreateFromUnderlyingArray(ref byte /// The underlying array, or null if is true. internal static byte[]? DangerousGetUnderlyingArray(ImmutableArray array) { - ByteArrayUnion union = default; - union.ImmutableArray = array; - return union.UnderlyingArray; - } - - [StructLayout(LayoutKind.Explicit)] - private struct ByteArrayUnion - { - [FieldOffset(0)] - internal byte[]? UnderlyingArray; - - [FieldOffset(0)] - internal ImmutableArray ImmutableArray; + return Unsafe.As, byte[]>(ref array); } } } From 417f538ccd2b14b7c30255823efdb7bac8662961 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Oct 2022 17:58:14 +0300 Subject: [PATCH 02/16] Optimize methods in the BlobUtilities class. --- .../Internal/Utilities/BlobUtilities.cs | 81 ++++++------------- 1 file changed, 23 insertions(+), 58 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs index ed3e2b07625b2a..90ece20e3e2de2 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs @@ -1,12 +1,11 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; +using System.Buffers.Binary; using System.Collections.Immutable; using System.Diagnostics; using System.Reflection.Internal; using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; namespace System.Reflection { @@ -14,44 +13,37 @@ internal static unsafe class BlobUtilities { public static byte[] ReadBytes(byte* buffer, int byteCount) { - if (byteCount == 0) - { - return Array.Empty(); - } - - byte[] result = new byte[byteCount]; - Marshal.Copy((IntPtr)buffer, result, 0, byteCount); - return result; + return new ReadOnlySpan(buffer, byteCount).ToArray(); } public static ImmutableArray ReadImmutableBytes(byte* buffer, int byteCount) { - byte[]? bytes = ReadBytes(buffer, byteCount); - return ImmutableByteArrayInterop.DangerousCreateFromUnderlyingArray(ref bytes); + return ImmutableArray.Create(new ReadOnlySpan(buffer, byteCount)); } public static void WriteBytes(this byte[] buffer, int start, byte value, int byteCount) { Debug.Assert(buffer.Length > 0); - fixed (byte* bufferPtr = &buffer[0]) - { - byte* startPtr = bufferPtr + start; - for (int i = 0; i < byteCount; i++) - { - startPtr[i] = value; - } - } + new Span(buffer, start, byteCount).Fill(value); } public static void WriteDouble(this byte[] buffer, int start, double value) { +#if NETCOREAPP + BinaryPrimitives.WriteDoubleLittleEndian(buffer.AsSpan(start), value); +#else WriteUInt64(buffer, start, *(ulong*)&value); +#endif } public static void WriteSingle(this byte[] buffer, int start, float value) { +#if NETCOREAPP + BinaryPrimitives.WriteSingleLittleEndian(buffer.AsSpan(start), value); +#else WriteUInt32(buffer, start, *(uint*)&value); +#endif } public static void WriteByte(this byte[] buffer, int start, byte value) @@ -62,60 +54,27 @@ public static void WriteByte(this byte[] buffer, int start, byte value) public static void WriteUInt16(this byte[] buffer, int start, ushort value) { - fixed (byte* ptr = &buffer[start]) - { - unchecked - { - ptr[0] = (byte)value; - ptr[1] = (byte)(value >> 8); - } - } + BinaryPrimitives.WriteUInt16LittleEndian(buffer.AsSpan(start), value); } public static void WriteUInt16BE(this byte[] buffer, int start, ushort value) { - fixed (byte* ptr = &buffer[start]) - { - unchecked - { - ptr[0] = (byte)(value >> 8); - ptr[1] = (byte)value; - } - } + BinaryPrimitives.WriteUInt16BigEndian(buffer.AsSpan(start), value); } public static void WriteUInt32BE(this byte[] buffer, int start, uint value) { - fixed (byte* ptr = &buffer[start]) - { - unchecked - { - ptr[0] = (byte)(value >> 24); - ptr[1] = (byte)(value >> 16); - ptr[2] = (byte)(value >> 8); - ptr[3] = (byte)value; - } - } + BinaryPrimitives.WriteUInt32BigEndian(buffer.AsSpan(start), value); } public static void WriteUInt32(this byte[] buffer, int start, uint value) { - fixed (byte* ptr = &buffer[start]) - { - unchecked - { - ptr[0] = (byte)value; - ptr[1] = (byte)(value >> 8); - ptr[2] = (byte)(value >> 16); - ptr[3] = (byte)(value >> 24); - } - } + BinaryPrimitives.WriteUInt32LittleEndian(buffer.AsSpan(start), value); } public static void WriteUInt64(this byte[] buffer, int start, ulong value) { - WriteUInt32(buffer, start, unchecked((uint)value)); - WriteUInt32(buffer, start + 4, unchecked((uint)(value >> 32))); + BinaryPrimitives.WriteUInt64LittleEndian(buffer.AsSpan(start), value); } public const int SizeOfSerializedDecimal = sizeof(byte) + 3 * sizeof(uint); @@ -137,6 +96,11 @@ public static void WriteDecimal(this byte[] buffer, int start, decimal value) public static void WriteGuid(this byte[] buffer, int start, Guid value) { +#if NETCOREAPP + bool written = value.TryWriteBytes(buffer.AsSpan(start)); + // This function is not public, callers have to ensure that enough space is available. + Debug.Assert(written); +#else fixed (byte* dst = &buffer[start]) { byte* src = (byte*)&value; @@ -167,6 +131,7 @@ public static void WriteGuid(this byte[] buffer, int start, Guid value) dst[14] = src[14]; dst[15] = src[15]; } +#endif } public static void WriteUTF8(this byte[] buffer, int start, char* charPtr, int charCount, int byteCount, bool allowUnpairedSurrogates) From ce584cdb3eabf12078a3756bbeedcd57ec7c52fc Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Oct 2022 18:04:33 +0300 Subject: [PATCH 03/16] Include more information in exceptions thrown from catch blocks. --- .../Internal/MemoryBlocks/StreamMemoryBlockProvider.cs | 5 +++-- .../Reflection/Metadata/MetadataReader.netstandard.cs | 2 +- .../PortableExecutable/PEReader.EmbeddedPortablePdb.cs | 6 +++--- .../src/System/Reflection/PortableExecutable/PEReader.cs | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamMemoryBlockProvider.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamMemoryBlockProvider.cs index 0d91893ac236cb..735ae1b182deeb 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamMemoryBlockProvider.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamMemoryBlockProvider.cs @@ -122,7 +122,7 @@ public override Stream GetStream(out StreamConstraints constraints) } /// IO error while mapping memory or not enough memory to create the mapping. - private unsafe bool TryCreateMemoryMappedFileBlock(long start, int size, [NotNullWhen(true)]out MemoryMappedFileBlock? block) + private unsafe bool TryCreateMemoryMappedFileBlock(long start, int size, [NotNullWhen(true)] out MemoryMappedFileBlock? block) { if (_lazyMemoryMap == null) { @@ -142,7 +142,8 @@ private unsafe bool TryCreateMemoryMappedFileBlock(long start, int size, [NotNul access: MemoryMappedFileAccess.Read, inheritability: HandleInheritability.None, leaveOpen: true); - } catch (UnauthorizedAccessException e) + } + catch (UnauthorizedAccessException e) { throw new IOException(e.Message, e); } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.netstandard.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.netstandard.cs index 49406b39dec156..a11a8fcacda4e5 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.netstandard.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.netstandard.cs @@ -97,7 +97,7 @@ public static unsafe AssemblyName GetAssemblyName(string assemblyFile) } catch (InvalidOperationException ex) { - throw new BadImageFormatException(ex.Message); + throw new BadImageFormatException(ex.Message, assemblyFile, ex); } } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.EmbeddedPortablePdb.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.EmbeddedPortablePdb.cs index 0c9d4450e2c52e..763f2b72a5dcb4 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.EmbeddedPortablePdb.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.EmbeddedPortablePdb.cs @@ -83,9 +83,9 @@ internal static unsafe NativeHeapMemoryBlock DecodeEmbeddedPortablePdbDebugDirec { decompressed = new NativeHeapMemoryBlock(decompressedSize); } - catch + catch (Exception e) { - throw new BadImageFormatException(SR.DataTooBig); + throw new BadImageFormatException(SR.DataTooBig, e); } bool success = false; @@ -110,7 +110,7 @@ internal static unsafe NativeHeapMemoryBlock DecodeEmbeddedPortablePdbDebugDirec } catch (Exception e) { - throw new BadImageFormatException(e.Message, e.InnerException); + throw new BadImageFormatException(e.Message, e); } if (actualLength != decompressed.Size) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.cs index eda470b5bd7137..43be6cc595f771 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.cs @@ -718,7 +718,7 @@ public bool TryOpenAssociatedPortablePdb(string peImagePath, Func Date: Mon, 3 Oct 2022 18:44:21 +0300 Subject: [PATCH 04/16] Use spans in BlobContentId, avoiding ImmutableByteArrayInterop. --- .../Reflection/Metadata/BlobContentId.cs | 36 +++++++++++++++---- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobContentId.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobContentId.cs index 653d9e69e61eca..7b05c00ab4c9a0 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobContentId.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobContentId.cs @@ -22,17 +22,27 @@ public BlobContentId(Guid guid, uint stamp) } public BlobContentId(ImmutableArray id) - : this(ImmutableByteArrayInterop.DangerousGetUnderlyingArray(id)!) { + if (id.IsDefault) + { + Throw.ArgumentNull(nameof(id)); + } + + (Guid, Stamp) = Initialize(id.AsSpan()); } - public unsafe BlobContentId(byte[] id) + public BlobContentId(byte[] id) { if (id is null) { Throw.ArgumentNull(nameof(id)); } + (Guid, Stamp) = Initialize(id); + } + + private static unsafe (Guid guid, uint stamp) Initialize(ReadOnlySpan id) + { if (id.Length != Size) { throw new ArgumentException(SR.Format(SR.UnexpectedArrayLength, Size), nameof(id)); @@ -41,8 +51,10 @@ public unsafe BlobContentId(byte[] id) fixed (byte* ptr = &id[0]) { var reader = new BlobReader(ptr, id.Length); - Guid = reader.ReadGuid(); - Stamp = reader.ReadUInt32(); + Guid guid = reader.ReadGuid(); + uint stamp = reader.ReadUInt32(); + + return (guid, stamp); } } @@ -50,7 +62,12 @@ public unsafe BlobContentId(byte[] id) public static BlobContentId FromHash(ImmutableArray hashCode) { - return FromHash(ImmutableByteArrayInterop.DangerousGetUnderlyingArray(hashCode)!); + if (hashCode.IsDefault) + { + Throw.ArgumentNull(nameof(hashCode)); + } + + return FromHash(hashCode.AsSpan()); } public static BlobContentId FromHash(byte[] hashCode) @@ -60,6 +77,11 @@ public static BlobContentId FromHash(byte[] hashCode) Throw.ArgumentNull(nameof(hashCode)); } + return FromHash(hashCode.AsSpan()); + } + + private static BlobContentId FromHash(ReadOnlySpan hashCode) + { const int minHashSize = 20; if (hashCode.Length < minHashSize) @@ -69,8 +91,8 @@ public static BlobContentId FromHash(byte[] hashCode) // extract guid components from input data uint a = ((uint)hashCode[3] << 24 | (uint)hashCode[2] << 16 | (uint)hashCode[1] << 8 | hashCode[0]); - ushort b = (ushort)((ushort)hashCode[5] << 8 | (ushort)hashCode[4]); - ushort c = (ushort)((ushort)hashCode[7] << 8 | (ushort)hashCode[6]); + ushort b = (ushort)(hashCode[5] << 8 | hashCode[4]); + ushort c = (ushort)(hashCode[7] << 8 | hashCode[6]); byte d = hashCode[8]; byte e = hashCode[9]; byte f = hashCode[10]; From a2acd50c51db545fa345543fb6d91b53b6c82a8f Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Oct 2022 19:36:43 +0300 Subject: [PATCH 05/16] Use spans in BlobWriter, reducing pinning and ImmutableArrayInterop. --- .../System/Reflection/Metadata/BlobWriter.cs | 102 ++++++++---------- 1 file changed, 43 insertions(+), 59 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs index dc63a0de0a557f..7ac52bd6b2d121 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs @@ -10,7 +10,7 @@ namespace System.Reflection.Metadata { // TODO: argument checking - public unsafe struct BlobWriter + public struct BlobWriter { // writable slice: private readonly byte[] _buffer; @@ -88,9 +88,7 @@ public byte[] ToArray(int start, int byteCount) { BlobUtilities.ValidateRange(Length, start, byteCount, nameof(byteCount)); - var result = new byte[byteCount]; - Array.Copy(_buffer, _start + start, result, 0, byteCount); - return result; + return _buffer.AsSpan(_start + start, byteCount).ToArray(); } public ImmutableArray ToImmutableArray() @@ -101,8 +99,9 @@ public ImmutableArray ToImmutableArray() /// Range specified by and falls outside of the bounds of the buffer content. public ImmutableArray ToImmutableArray(int start, int byteCount) { - byte[]? array = ToArray(start, byteCount); - return ImmutableByteArrayInterop.DangerousCreateFromUnderlyingArray(ref array); + BlobUtilities.ValidateRange(Length, start, byteCount, nameof(byteCount)); + + return ImmutableArray.Create(_buffer.AsSpan(_start + start, byteCount)); } private int Advance(int value) @@ -128,14 +127,7 @@ public void WriteBytes(byte value, int byteCount) } int start = Advance(byteCount); - fixed (byte* buffer = _buffer) - { - byte* ptr = buffer + start; - for (int i = 0; i < byteCount; i++) - { - ptr[i] = value; - } - } + _buffer.AsSpan(start, byteCount).Fill(value); } /// is null. @@ -152,13 +144,13 @@ public unsafe void WriteBytes(byte* buffer, int byteCount) Throw.ArgumentOutOfRange(nameof(byteCount)); } - WriteBytesUnchecked(buffer, byteCount); + WriteBytesUnchecked(new ReadOnlySpan(buffer, byteCount)); } - private unsafe void WriteBytesUnchecked(byte* buffer, int byteCount) + private void WriteBytesUnchecked(ReadOnlySpan buffer) { - int start = Advance(byteCount); - Marshal.Copy((IntPtr)buffer, _buffer, start, byteCount); + int start = Advance(buffer.Length); + buffer.CopyTo(_buffer.AsSpan(start)); } /// is null. @@ -195,25 +187,42 @@ public int WriteBytes(Stream source, int byteCount) /// is null. public void WriteBytes(ImmutableArray buffer) { - WriteBytes(buffer, 0, buffer.IsDefault ? 0 : buffer.Length); + if (buffer.IsDefault) + { + Throw.ArgumentNull(nameof(buffer)); + } + + WriteBytesUnchecked(buffer.AsSpan()); } /// is null. /// Range specified by and falls outside of the bounds of the . public void WriteBytes(ImmutableArray buffer, int start, int byteCount) { - WriteBytes(ImmutableByteArrayInterop.DangerousGetUnderlyingArray(buffer)!, start, byteCount); + if (buffer.IsDefault) + { + Throw.ArgumentNull(nameof(buffer)); + } + + BlobUtilities.ValidateRange(buffer.Length, start, byteCount, nameof(byteCount)); + + WriteBytesUnchecked(buffer.AsSpan(start, byteCount)); } /// is null. - public unsafe void WriteBytes(byte[] buffer) + public void WriteBytes(byte[] buffer) { - WriteBytes(buffer, 0, buffer?.Length ?? 0); + if (buffer is null) + { + Throw.ArgumentNull(nameof(buffer)); + } + + WriteBytesUnchecked(buffer); } /// is null. /// Range specified by and falls outside of the bounds of the . - public unsafe void WriteBytes(byte[] buffer, int start, int byteCount) + public void WriteBytes(byte[] buffer, int start, int byteCount) { if (buffer is null) { @@ -222,16 +231,7 @@ public unsafe void WriteBytes(byte[] buffer, int start, int byteCount) BlobUtilities.ValidateRange(buffer.Length, start, byteCount, nameof(byteCount)); - // an empty array has no element pointer: - if (buffer.Length == 0) - { - return; - } - - fixed (byte* ptr = &buffer[0]) - { - WriteBytes(ptr + start, byteCount); - } + WriteBytesUnchecked(buffer.AsSpan(start, byteCount)); } public void PadTo(int offset) @@ -376,25 +376,7 @@ public void WriteUTF16(char[] value) Throw.ArgumentNull(nameof(value)); } - if (value.Length == 0) - { - return; - } - - if (BitConverter.IsLittleEndian) - { - fixed (char* ptr = &value[0]) - { - WriteBytesUnchecked((byte*)ptr, value.Length * sizeof(char)); - } - } - else - { - for (int i = 0; i < value.Length; i++) - { - WriteUInt16((ushort)value[i]); - } - } + WriteUTF16(value.AsSpan()); } /// @@ -408,18 +390,20 @@ public void WriteUTF16(string value) Throw.ArgumentNull(nameof(value)); } + WriteUTF16(value.AsSpan()); + } + + private void WriteUTF16(ReadOnlySpan value) + { if (BitConverter.IsLittleEndian) { - fixed (char* ptr = value) - { - WriteBytesUnchecked((byte*)ptr, value.Length * sizeof(char)); - } + WriteBytesUnchecked(MemoryMarshal.AsBytes(value)); } else { - for (int i = 0; i < value.Length; i++) + foreach (char c in value) { - WriteUInt16((ushort)value[i]); + WriteUInt16(c); } } } @@ -480,7 +464,7 @@ public void WriteUTF8(string value, bool allowUnpairedSurrogates) WriteUTF8(value, 0, value.Length, allowUnpairedSurrogates, prependSize: false); } - private void WriteUTF8(string str, int start, int length, bool allowUnpairedSurrogates, bool prependSize) + private unsafe void WriteUTF8(string str, int start, int length, bool allowUnpairedSurrogates, bool prependSize) { fixed (char* strPtr = str) { From daed1793163b4b72f52f3f2cfecce580e11819e9 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Oct 2022 19:41:04 +0300 Subject: [PATCH 06/16] Use spans in BlobBuilder, reducing pinning and ImmutableArrayInterop. --- .../System/Reflection/Metadata/BlobBuilder.cs | 110 +++++++++--------- 1 file changed, 58 insertions(+), 52 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs index 6cde1154c5703c..00a8923b7f0e12 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs @@ -7,7 +7,6 @@ using System.Reflection.Internal; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; -using System.Text; namespace System.Reflection.Metadata { @@ -360,7 +359,7 @@ public void WriteContentTo(BlobBuilder destination) foreach (var chunk in GetChunks()) { - destination.WriteBytes(chunk._buffer, 0, chunk.Length); + destination.WriteBytes(chunk._buffer.AsSpan()); } } @@ -652,24 +651,29 @@ public unsafe void WriteBytes(byte* buffer, int byteCount) Throw.InvalidOperationBuilderAlreadyLinked(); } - WriteBytesUnchecked(buffer, byteCount); + WriteBytesUnchecked(new ReadOnlySpan(buffer, byteCount)); } - private unsafe void WriteBytesUnchecked(byte* buffer, int byteCount) + private void WriteBytesUnchecked(ReadOnlySpan buffer) { - int bytesToCurrent = Math.Min(FreeBytes, byteCount); + if (buffer.IsEmpty) + { + return; + } + + int bytesToCurrent = Math.Min(FreeBytes, buffer.Length); - Marshal.Copy((IntPtr)buffer, _buffer, Length, bytesToCurrent); + buffer.Slice(0, bytesToCurrent).CopyTo(_buffer.AsSpan(Length)); AddLength(bytesToCurrent); - int remaining = byteCount - bytesToCurrent; - if (remaining > 0) + ReadOnlySpan remaining = buffer.Slice(bytesToCurrent); + if (!remaining.IsEmpty) { - Expand(remaining); + Expand(remaining.Length); - Marshal.Copy((IntPtr)(buffer + bytesToCurrent), _buffer, 0, remaining); - AddLength(remaining); + remaining.CopyTo(_buffer); + AddLength(remaining.Length); } } @@ -725,7 +729,12 @@ public int TryWriteBytes(Stream source, int byteCount) /// Builder is not writable, it has been linked with another one. public void WriteBytes(ImmutableArray buffer) { - WriteBytes(buffer, 0, buffer.IsDefault ? 0 : buffer.Length); + if (buffer.IsDefault) + { + Throw.ArgumentNull(nameof(buffer)); + } + + WriteBytes(buffer.AsSpan()); } /// is null. @@ -733,20 +742,32 @@ public void WriteBytes(ImmutableArray buffer) /// Builder is not writable, it has been linked with another one. public void WriteBytes(ImmutableArray buffer, int start, int byteCount) { - WriteBytes(ImmutableByteArrayInterop.DangerousGetUnderlyingArray(buffer)!, start, byteCount); + if (buffer.IsDefault) + { + Throw.ArgumentNull(nameof(buffer)); + } + + BlobUtilities.ValidateRange(buffer.Length, start, byteCount, nameof(byteCount)); + + WriteBytes(buffer.AsSpan(start, byteCount)); } /// is null. /// Builder is not writable, it has been linked with another one. public void WriteBytes(byte[] buffer) { - WriteBytes(buffer, 0, buffer?.Length ?? 0); + if (buffer is null) + { + Throw.ArgumentNull(nameof(buffer)); + } + + WriteBytes(buffer.AsSpan()); } /// is null. /// Range specified by and falls outside of the bounds of the . /// Builder is not writable, it has been linked with another one. - public unsafe void WriteBytes(byte[] buffer, int start, int byteCount) + public void WriteBytes(byte[] buffer, int start, int byteCount) { if (buffer is null) { @@ -755,21 +776,17 @@ public unsafe void WriteBytes(byte[] buffer, int start, int byteCount) BlobUtilities.ValidateRange(buffer.Length, start, byteCount, nameof(byteCount)); + WriteBytes(buffer.AsSpan(start, byteCount)); + } + + private void WriteBytes(ReadOnlySpan buffer) + { if (!IsHead) { Throw.InvalidOperationBuilderAlreadyLinked(); } - // an empty array has no element pointer: - if (buffer.Length == 0) - { - return; - } - - fixed (byte* ptr = &buffer[0]) - { - WriteBytesUnchecked(ptr + start, byteCount); - } + WriteBytesUnchecked(buffer); } /// Builder is not writable, it has been linked with another one. @@ -929,7 +946,7 @@ public void WriteReference(int reference, bool isSmall) /// /// is null. /// Builder is not writable, it has been linked with another one. - public unsafe void WriteUTF16(char[] value) + public void WriteUTF16(char[] value) { if (value is null) { @@ -941,25 +958,7 @@ public unsafe void WriteUTF16(char[] value) Throw.InvalidOperationBuilderAlreadyLinked(); } - if (value.Length == 0) - { - return; - } - - if (BitConverter.IsLittleEndian) - { - fixed (char* ptr = &value[0]) - { - WriteBytesUnchecked((byte*)ptr, value.Length * sizeof(char)); - } - } - else - { - for (int i = 0; i < value.Length; i++) - { - WriteUInt16((ushort)value[i]); - } - } + WriteUTF16(value.AsSpan()); } /// @@ -967,7 +966,7 @@ public unsafe void WriteUTF16(char[] value) /// /// is null. /// Builder is not writable, it has been linked with another one. - public unsafe void WriteUTF16(string value) + public void WriteUTF16(string value) { if (value is null) { @@ -979,18 +978,25 @@ public unsafe void WriteUTF16(string value) Throw.InvalidOperationBuilderAlreadyLinked(); } + WriteUTF16(value.AsSpan()); + } + + private void WriteUTF16(ReadOnlySpan value) + { + if (!IsHead) + { + Throw.InvalidOperationBuilderAlreadyLinked(); + } + if (BitConverter.IsLittleEndian) { - fixed (char* ptr = value) - { - WriteBytesUnchecked((byte*)ptr, value.Length * sizeof(char)); - } + WriteBytesUnchecked(MemoryMarshal.AsBytes(value)); } else { - for (int i = 0; i < value.Length; i++) + foreach (char c in value) { - WriteUInt16((ushort)value[i]); + WriteUInt16(c); } } } From 47350a72cb0956f005c273bd7d736f108bb946c7 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 3 Oct 2022 19:45:16 +0300 Subject: [PATCH 07/16] Remove NoInlining from the throw helpers. It prevents the JIT from looking into the method's body, realizing it's a throw helper, and doing what's best (such as considering it cold). --- .../src/System/Reflection/Throw.cs | 43 ------------------- 1 file changed, 43 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Throw.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Throw.cs index b562a57626da45..ff98271852d0cb 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Throw.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Throw.cs @@ -4,7 +4,6 @@ using System.Diagnostics.CodeAnalysis; using System.Reflection.Metadata; using System.Reflection.Metadata.Ecma335; -using System.Runtime.CompilerServices; namespace System.Reflection { @@ -13,294 +12,252 @@ namespace System.Reflection internal static class Throw { [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidCast() { throw new InvalidCastException(); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidArgument(string message, string parameterName) { throw new ArgumentException(message, parameterName); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidArgument_OffsetForVirtualHeapHandle() { throw new ArgumentException(SR.CantGetOffsetForVirtualHeapHandle, "handle"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static Exception InvalidArgument_UnexpectedHandleKind(HandleKind kind) { throw new ArgumentException(SR.Format(SR.UnexpectedHandleKind, kind)); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static Exception InvalidArgument_Handle(string parameterName) { throw new ArgumentException(SR.InvalidHandle, parameterName); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void SignatureNotVarArg() { throw new InvalidOperationException(SR.SignatureNotVarArg); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ControlFlowBuilderNotAvailable() { throw new InvalidOperationException(SR.ControlFlowBuilderNotAvailable); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidOperationBuilderAlreadyLinked() { throw new InvalidOperationException(SR.BuilderAlreadyLinked); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidOperation(string message) { throw new InvalidOperationException(message); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidOperation_LabelNotMarked(int id) { throw new InvalidOperationException(SR.Format(SR.LabelNotMarked, id)); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void LabelDoesntBelongToBuilder(string parameterName) { throw new ArgumentException(SR.LabelDoesntBelongToBuilder, parameterName); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void HeapHandleRequired() { throw new ArgumentException(SR.NotMetadataHeapHandle, "handle"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void EntityOrUserStringHandleRequired() { throw new ArgumentException(SR.NotMetadataTableOrUserStringHandle, "handle"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidToken() { throw new ArgumentException(SR.InvalidToken, "token"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ArgumentNull(string parameterName) { throw new ArgumentNullException(parameterName); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ArgumentEmptyString(string parameterName) { throw new ArgumentException(SR.ExpectedNonEmptyString, parameterName); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ArgumentEmptyArray(string parameterName) { throw new ArgumentException(SR.ExpectedNonEmptyArray, parameterName); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ValueArgumentNull() { throw new ArgumentNullException("value"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void BuilderArgumentNull() { throw new ArgumentNullException("builder"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ArgumentOutOfRange(string parameterName) { throw new ArgumentOutOfRangeException(parameterName); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ArgumentOutOfRange(string parameterName, string message) { throw new ArgumentOutOfRangeException(parameterName, message); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void BlobTooLarge(string parameterName) { throw new ArgumentOutOfRangeException(parameterName, SR.BlobTooLarge); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void IndexOutOfRange() { throw new ArgumentOutOfRangeException("index"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void TableIndexOutOfRange() { throw new ArgumentOutOfRangeException("tableIndex"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ValueArgumentOutOfRange() { throw new ArgumentOutOfRangeException("value"); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void OutOfBounds() { throw new BadImageFormatException(SR.OutOfBoundsRead); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void WriteOutOfBounds() { throw new InvalidOperationException(SR.OutOfBoundsWrite); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidCodedIndex() { throw new BadImageFormatException(SR.InvalidCodedIndex); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidHandle() { throw new BadImageFormatException(SR.InvalidHandle); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidCompressedInteger() { throw new BadImageFormatException(SR.InvalidCompressedInteger); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidSerializedString() { throw new BadImageFormatException(SR.InvalidSerializedString); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ImageTooSmall() { throw new BadImageFormatException(SR.ImageTooSmall); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ImageTooSmallOrContainsInvalidOffsetOrCount() { throw new BadImageFormatException(SR.ImageTooSmallOrContainsInvalidOffsetOrCount); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ReferenceOverflow() { throw new BadImageFormatException(SR.RowIdOrHeapOffsetTooLarge); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void TableNotSorted(TableIndex tableIndex) { throw new BadImageFormatException(SR.Format(SR.MetadataTableNotSorted, tableIndex)); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidOperation_TableNotSorted(TableIndex tableIndex) { throw new InvalidOperationException(SR.Format(SR.MetadataTableNotSorted, tableIndex)); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void InvalidOperation_PEImageNotAvailable() { throw new InvalidOperationException(SR.PEImageNotAvailable); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void TooManySubnamespaces() { throw new BadImageFormatException(SR.TooManySubnamespaces); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void ValueOverflow() { throw new BadImageFormatException(SR.ValueTooLarge); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void SequencePointValueOutOfRange() { throw new BadImageFormatException(SR.SequencePointValueOutOfRange); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void HeapSizeLimitExceeded(HeapIndex heap) { throw new ImageFormatLimitationException(SR.Format(SR.HeapSizeLimitExceeded, heap)); } [DoesNotReturn] - [MethodImpl(MethodImplOptions.NoInlining)] internal static void PEReaderDisposed() { throw new ObjectDisposedException(nameof(PortableExecutable.PEReader)); From 9a56ab1f706c6d8ffce78f5ab18e9319883c7116 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 8 Oct 2022 01:02:48 +0300 Subject: [PATCH 08/16] Inline `BlobUtilities.Read(Immutable)?Bytes`. --- .../Internal/MemoryBlocks/AbstractMemoryBlock.cs | 2 +- .../Reflection/Internal/Utilities/BlobUtilities.cs | 11 ----------- .../Reflection/Internal/Utilities/MemoryBlock.cs | 2 +- .../tests/Utilities/StreamExtensionsTests.cs | 2 +- 4 files changed, 3 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/AbstractMemoryBlock.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/AbstractMemoryBlock.cs index c53809dec36cab..cee648185fbfdc 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/AbstractMemoryBlock.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/AbstractMemoryBlock.cs @@ -34,7 +34,7 @@ internal abstract class AbstractMemoryBlock : IDisposable /// public virtual unsafe ImmutableArray GetContentUnchecked(int start, int length) { - var result = BlobUtilities.ReadImmutableBytes(Pointer + start, length); + var result = new ReadOnlySpan(Pointer + start, length).ToImmutableArray(); GC.KeepAlive(this); return result; } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs index 90ece20e3e2de2..f0829bac3d2a25 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Buffers.Binary; -using System.Collections.Immutable; using System.Diagnostics; using System.Reflection.Internal; using System.Runtime.CompilerServices; @@ -11,16 +10,6 @@ namespace System.Reflection { internal static unsafe class BlobUtilities { - public static byte[] ReadBytes(byte* buffer, int byteCount) - { - return new ReadOnlySpan(buffer, byteCount).ToArray(); - } - - public static ImmutableArray ReadImmutableBytes(byte* buffer, int byteCount) - { - return ImmutableArray.Create(new ReadOnlySpan(buffer, byteCount)); - } - public static void WriteBytes(this byte[] buffer, int start, byte value, int byteCount) { Debug.Assert(buffer.Length > 0); diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/MemoryBlock.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/MemoryBlock.cs index 5931cb2043ab9e..7de2e008a353d8 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/MemoryBlock.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/MemoryBlock.cs @@ -512,7 +512,7 @@ internal int CompareUtf8NullTerminatedStringWithAsciiString(int offset, string a internal byte[] PeekBytes(int offset, int byteCount) { CheckBounds(offset, byteCount); - return BlobUtilities.ReadBytes(Pointer + offset, byteCount); + return new ReadOnlySpan(Pointer + offset, byteCount).ToArray(); } internal int IndexOf(byte b, int start) diff --git a/src/libraries/System.Reflection.Metadata/tests/Utilities/StreamExtensionsTests.cs b/src/libraries/System.Reflection.Metadata/tests/Utilities/StreamExtensionsTests.cs index 20e0094a1ebd0b..09e56563407fa7 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Utilities/StreamExtensionsTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Utilities/StreamExtensionsTests.cs @@ -25,7 +25,7 @@ private static unsafe byte[] ReadBuffer(byte* buffer, int bufferSize) p--; } - return BlobUtilities.ReadBytes(buffer, (int)(p + 1 - buffer)); + return new ReadOnlySpan(buffer, (int)(p + 1 - buffer)).ToArray(); } [Fact] From 8ebbbfa6cd59e759dbd0495465e982b54311b737 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 8 Oct 2022 17:09:06 +0300 Subject: [PATCH 09/16] Avoid a length check introduced by the use of `BinaryPrimitives`. --- .../Internal/Utilities/BlobUtilities.cs | 39 ++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs index f0829bac3d2a25..0db2e4cc212a0e 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using System.Reflection.Internal; using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; namespace System.Reflection { @@ -19,20 +20,12 @@ public static void WriteBytes(this byte[] buffer, int start, byte value, int byt public static void WriteDouble(this byte[] buffer, int start, double value) { -#if NETCOREAPP - BinaryPrimitives.WriteDoubleLittleEndian(buffer.AsSpan(start), value); -#else WriteUInt64(buffer, start, *(ulong*)&value); -#endif } public static void WriteSingle(this byte[] buffer, int start, float value) { -#if NETCOREAPP - BinaryPrimitives.WriteSingleLittleEndian(buffer.AsSpan(start), value); -#else WriteUInt32(buffer, start, *(uint*)&value); -#endif } public static void WriteByte(this byte[] buffer, int start, byte value) @@ -43,27 +36,47 @@ public static void WriteByte(this byte[] buffer, int start, byte value) public static void WriteUInt16(this byte[] buffer, int start, ushort value) { - BinaryPrimitives.WriteUInt16LittleEndian(buffer.AsSpan(start), value); + if (!BitConverter.IsLittleEndian) + { + value = BinaryPrimitives.ReverseEndianness(value); + } + Unsafe.WriteUnaligned(ref buffer[start], value); } public static void WriteUInt16BE(this byte[] buffer, int start, ushort value) { - BinaryPrimitives.WriteUInt16BigEndian(buffer.AsSpan(start), value); + if (BitConverter.IsLittleEndian) + { + value = BinaryPrimitives.ReverseEndianness(value); + } + Unsafe.WriteUnaligned(ref buffer[start], value); } public static void WriteUInt32BE(this byte[] buffer, int start, uint value) { - BinaryPrimitives.WriteUInt32BigEndian(buffer.AsSpan(start), value); + if (BitConverter.IsLittleEndian) + { + value = BinaryPrimitives.ReverseEndianness(value); + } + Unsafe.WriteUnaligned(ref buffer[start], value); } public static void WriteUInt32(this byte[] buffer, int start, uint value) { - BinaryPrimitives.WriteUInt32LittleEndian(buffer.AsSpan(start), value); + if (!BitConverter.IsLittleEndian) + { + value = BinaryPrimitives.ReverseEndianness(value); + } + Unsafe.WriteUnaligned(ref buffer[start], value); } public static void WriteUInt64(this byte[] buffer, int start, ulong value) { - BinaryPrimitives.WriteUInt64LittleEndian(buffer.AsSpan(start), value); + if (!BitConverter.IsLittleEndian) + { + value = BinaryPrimitives.ReverseEndianness(value); + } + Unsafe.WriteUnaligned(ref buffer[start], value); } public const int SizeOfSerializedDecimal = sizeof(byte) + 3 * sizeof(uint); From 85bc2d603c0aa1396eae84e233dc8f7a60cd3fd2 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 8 Oct 2022 17:09:58 +0300 Subject: [PATCH 10/16] Remove `DisallowNull` from `ImmutableArrayInterop.DangerousCreateFromUnderlyingArray`. --- .../Internal/Utilities/ImmutableByteArrayInterop.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs index 7fb48d0d26feaa..062133428d15d7 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs @@ -49,9 +49,9 @@ internal static unsafe class ImmutableByteArrayInterop /// whose underlying backing field is modified. /// /// An immutable array. - internal static ImmutableArray DangerousCreateFromUnderlyingArray([DisallowNull] ref byte[]? array) + internal static ImmutableArray DangerousCreateFromUnderlyingArray(ref byte[]? array) { - byte[] givenArray = array; + byte[] givenArray = array!; array = null; return Unsafe.As>(ref givenArray); From de0c6f24ee84f8cea20bb4edd5d29cabe3c5a642 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sat, 8 Oct 2022 17:13:09 +0300 Subject: [PATCH 11/16] Remove an early bail-out if the buffer was empty. --- .../src/System/Reflection/Metadata/BlobBuilder.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs index 00a8923b7f0e12..88502015bbd36e 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs @@ -656,11 +656,6 @@ public unsafe void WriteBytes(byte* buffer, int byteCount) private void WriteBytesUnchecked(ReadOnlySpan buffer) { - if (buffer.IsEmpty) - { - return; - } - int bytesToCurrent = Math.Min(FreeBytes, buffer.Length); buffer.Slice(0, bytesToCurrent).CopyTo(_buffer.AsSpan(Length)); From 5001e7fa9e3da92f8840ef472fd8ef6170baea58 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 13 Oct 2022 01:29:47 +0300 Subject: [PATCH 12/16] Stop using value tuples. --- .../Reflection/Metadata/BlobContentId.cs | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobContentId.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobContentId.cs index 7b05c00ab4c9a0..db433de1e75940 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobContentId.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobContentId.cs @@ -12,13 +12,16 @@ namespace System.Reflection.Metadata { private const int Size = BlobUtilities.SizeOfGuid + sizeof(uint); - public Guid Guid { get; } - public uint Stamp { get; } + private readonly Guid _guid; + private readonly uint _stamp; + + public Guid Guid => _guid; + public uint Stamp => _stamp; public BlobContentId(Guid guid, uint stamp) { - Guid = guid; - Stamp = stamp; + _guid = guid; + _stamp = stamp; } public BlobContentId(ImmutableArray id) @@ -28,7 +31,7 @@ public BlobContentId(ImmutableArray id) Throw.ArgumentNull(nameof(id)); } - (Guid, Stamp) = Initialize(id.AsSpan()); + Initialize(id.AsSpan(), out _guid, out _stamp); } public BlobContentId(byte[] id) @@ -38,10 +41,10 @@ public BlobContentId(byte[] id) Throw.ArgumentNull(nameof(id)); } - (Guid, Stamp) = Initialize(id); + Initialize(id, out _guid, out _stamp); } - private static unsafe (Guid guid, uint stamp) Initialize(ReadOnlySpan id) + private static unsafe void Initialize(ReadOnlySpan id, out Guid guid, out uint stamp) { if (id.Length != Size) { @@ -51,10 +54,8 @@ private static unsafe (Guid guid, uint stamp) Initialize(ReadOnlySpan id) fixed (byte* ptr = &id[0]) { var reader = new BlobReader(ptr, id.Length); - Guid guid = reader.ReadGuid(); - uint stamp = reader.ReadUInt32(); - - return (guid, stamp); + guid = reader.ReadGuid(); + stamp = reader.ReadUInt32(); } } From 94b72c1771438551d8f309d29af3b77a8f9db97b Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 13 Oct 2022 01:37:58 +0300 Subject: [PATCH 13/16] Fix tests and expose writing a span to BlobWriter. --- .../src/System/Reflection/Metadata/BlobBuilder.cs | 4 ++-- .../src/System/Reflection/Metadata/BlobWriter.cs | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs index 88502015bbd36e..8debffc0e94761 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobBuilder.cs @@ -344,7 +344,7 @@ public void WriteContentTo(ref BlobWriter destination) foreach (var chunk in GetChunks()) { - destination.WriteBytes(chunk._buffer, 0, chunk.Length); + destination.WriteBytes(chunk._buffer.AsSpan(0, chunk.Length)); } } @@ -359,7 +359,7 @@ public void WriteContentTo(BlobBuilder destination) foreach (var chunk in GetChunks()) { - destination.WriteBytes(chunk._buffer.AsSpan()); + destination.WriteBytes(chunk._buffer.AsSpan(0, chunk.Length)); } } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs index 7ac52bd6b2d121..0c61f1df4ee089 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs @@ -144,10 +144,10 @@ public unsafe void WriteBytes(byte* buffer, int byteCount) Throw.ArgumentOutOfRange(nameof(byteCount)); } - WriteBytesUnchecked(new ReadOnlySpan(buffer, byteCount)); + WriteBytes(new ReadOnlySpan(buffer, byteCount)); } - private void WriteBytesUnchecked(ReadOnlySpan buffer) + internal void WriteBytes(ReadOnlySpan buffer) { int start = Advance(buffer.Length); buffer.CopyTo(_buffer.AsSpan(start)); @@ -192,7 +192,7 @@ public void WriteBytes(ImmutableArray buffer) Throw.ArgumentNull(nameof(buffer)); } - WriteBytesUnchecked(buffer.AsSpan()); + WriteBytes(buffer.AsSpan()); } /// is null. @@ -206,7 +206,7 @@ public void WriteBytes(ImmutableArray buffer, int start, int byteCount) BlobUtilities.ValidateRange(buffer.Length, start, byteCount, nameof(byteCount)); - WriteBytesUnchecked(buffer.AsSpan(start, byteCount)); + WriteBytes(buffer.AsSpan(start, byteCount)); } /// is null. @@ -217,7 +217,7 @@ public void WriteBytes(byte[] buffer) Throw.ArgumentNull(nameof(buffer)); } - WriteBytesUnchecked(buffer); + WriteBytes(buffer); } /// is null. @@ -231,7 +231,7 @@ public void WriteBytes(byte[] buffer, int start, int byteCount) BlobUtilities.ValidateRange(buffer.Length, start, byteCount, nameof(byteCount)); - WriteBytesUnchecked(buffer.AsSpan(start, byteCount)); + WriteBytes(buffer.AsSpan(start, byteCount)); } public void PadTo(int offset) @@ -397,7 +397,7 @@ private void WriteUTF16(ReadOnlySpan value) { if (BitConverter.IsLittleEndian) { - WriteBytesUnchecked(MemoryMarshal.AsBytes(value)); + WriteBytes(MemoryMarshal.AsBytes(value)); } else { From a4b647c42145a309b0fd6c7a17429aaa54f0defb Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 13 Oct 2022 12:54:49 +0300 Subject: [PATCH 14/16] Fix stack overflows. --- .../src/System/Reflection/Metadata/BlobWriter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs index 0c61f1df4ee089..fdd73e33ba289a 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriter.cs @@ -217,7 +217,7 @@ public void WriteBytes(byte[] buffer) Throw.ArgumentNull(nameof(buffer)); } - WriteBytes(buffer); + WriteBytes(buffer.AsSpan()); } /// is null. From d466b383fd79f67f9e164f7ef0167f5e38123a7e Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 27 Oct 2022 22:46:35 +0300 Subject: [PATCH 15/16] Shorten BlobUtilities methods that write integers. --- .../Internal/Utilities/BlobUtilities.cs | 50 ++++--------------- 1 file changed, 10 insertions(+), 40 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs index 0db2e4cc212a0e..8f3a1321c284c6 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs @@ -34,50 +34,20 @@ public static void WriteByte(this byte[] buffer, int start, byte value) buffer[start] = value; } - public static void WriteUInt16(this byte[] buffer, int start, ushort value) - { - if (!BitConverter.IsLittleEndian) - { - value = BinaryPrimitives.ReverseEndianness(value); - } - Unsafe.WriteUnaligned(ref buffer[start], value); - } + public static void WriteUInt16(this byte[] buffer, int start, ushort value) => + Unsafe.WriteUnaligned(ref buffer[start], !BitConverter.IsLittleEndian ? BinaryPrimitives.ReverseEndianness(value) : value); - public static void WriteUInt16BE(this byte[] buffer, int start, ushort value) - { - if (BitConverter.IsLittleEndian) - { - value = BinaryPrimitives.ReverseEndianness(value); - } - Unsafe.WriteUnaligned(ref buffer[start], value); - } + public static void WriteUInt16BE(this byte[] buffer, int start, ushort value) => + Unsafe.WriteUnaligned(ref buffer[start], BitConverter.IsLittleEndian ? BinaryPrimitives.ReverseEndianness(value) : value); - public static void WriteUInt32BE(this byte[] buffer, int start, uint value) - { - if (BitConverter.IsLittleEndian) - { - value = BinaryPrimitives.ReverseEndianness(value); - } - Unsafe.WriteUnaligned(ref buffer[start], value); - } + public static void WriteUInt32BE(this byte[] buffer, int start, uint value) => + Unsafe.WriteUnaligned(ref buffer[start], BitConverter.IsLittleEndian ? BinaryPrimitives.ReverseEndianness(value) : value); - public static void WriteUInt32(this byte[] buffer, int start, uint value) - { - if (!BitConverter.IsLittleEndian) - { - value = BinaryPrimitives.ReverseEndianness(value); - } - Unsafe.WriteUnaligned(ref buffer[start], value); - } + public static void WriteUInt32(this byte[] buffer, int start, uint value) => + Unsafe.WriteUnaligned(ref buffer[start], !BitConverter.IsLittleEndian ? BinaryPrimitives.ReverseEndianness(value) : value); - public static void WriteUInt64(this byte[] buffer, int start, ulong value) - { - if (!BitConverter.IsLittleEndian) - { - value = BinaryPrimitives.ReverseEndianness(value); - } - Unsafe.WriteUnaligned(ref buffer[start], value); - } + public static void WriteUInt64(this byte[] buffer, int start, ulong value) => + Unsafe.WriteUnaligned(ref buffer[start], !BitConverter.IsLittleEndian ? BinaryPrimitives.ReverseEndianness(value) : value); public const int SizeOfSerializedDecimal = sizeof(byte) + 3 * sizeof(uint); From bc43334baeb01b529e85fcd83cf7931f60d63192 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Fri, 28 Oct 2022 14:58:35 +0300 Subject: [PATCH 16/16] Optimize some methods in `MemoryBlock`. --- .../Internal/Utilities/MemoryBlock.cs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/MemoryBlock.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/MemoryBlock.cs index 7de2e008a353d8..64326d2bce4fa3 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/MemoryBlock.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/MemoryBlock.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.Buffers.Binary; using System.Diagnostics; using System.Reflection.Metadata; using System.Reflection.Metadata.Ecma335; @@ -126,11 +127,8 @@ internal uint PeekUInt32(int offset) { CheckBounds(offset, sizeof(uint)); - unchecked - { - byte* ptr = Pointer + offset; - return (uint)(ptr[0] | (ptr[1] << 8) | (ptr[2] << 16) | (ptr[3] << 24)); - } + uint result = Unsafe.ReadUnaligned(Pointer + offset); + return BitConverter.IsLittleEndian ? result : BinaryPrimitives.ReverseEndianness(result); } /// @@ -187,11 +185,8 @@ internal ushort PeekUInt16(int offset) { CheckBounds(offset, sizeof(ushort)); - unchecked - { - byte* ptr = Pointer + offset; - return (ushort)(ptr[0] | (ptr[1] << 8)); - } + ushort result = Unsafe.ReadUnaligned(Pointer + offset); + return BitConverter.IsLittleEndian ? result : BinaryPrimitives.ReverseEndianness(result); } // When reference has tag bits. @@ -250,7 +245,7 @@ internal Guid PeekGuid(int offset) byte* ptr = Pointer + offset; if (BitConverter.IsLittleEndian) { - return *(Guid*)ptr; + return Unsafe.ReadUnaligned(ptr); } else {