From 47aa264f8fddf26c3afffe5259fca539e795be2c Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Tue, 6 Nov 2018 11:02:16 -0800 Subject: [PATCH] Improve performance of Memory.Span property getter (dotnet/coreclr#20386) - We can use our knowledge of object representation in the runtime to speed up type checks. - We leave the ref T and the length deconstructed until the very end, optimizing register usage. - The Length property getter is once again just a simple field accessor with no bitwise logic. Commit migrated from https://github.com/dotnet/coreclr/commit/ef93a727984dbc5b8925a0c2d723be6580d20460 --- .../Runtime/CompilerServices/RuntimeHelpers.cs | 43 +++- src/coreclr/tests/CoreFX/CoreFX.issues.json | 8 + .../System.Private.CoreLib/src/System/Memory.cs | 230 ++++++++++++--------- .../src/System/ReadOnlyMemory.cs | 194 ++++++++++------- .../Runtime/InteropServices/MemoryMarshal.cs | 48 +++-- 5 files changed, 345 insertions(+), 178 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs index 53b2848..93490d9 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs @@ -13,6 +13,7 @@ namespace System.Runtime.CompilerServices { using System; + using System.Diagnostics; using System.Security; using System.Runtime; using System.Runtime.CompilerServices; @@ -21,6 +22,7 @@ namespace System.Runtime.CompilerServices using System.Runtime.Serialization; using System.Threading; using System.Runtime.Versioning; + using Internal.Runtime.CompilerServices; public static class RuntimeHelpers { @@ -194,6 +196,45 @@ namespace System.Runtime.CompilerServices // See getILIntrinsicImplementation for how this happens. throw new InvalidOperationException(); } + + // Returns true iff the object has a component size; + // i.e., is variable length like System.String or Array. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal static unsafe bool ObjectHasComponentSize(object obj) + { + // CLR objects are laid out in memory as follows. + // [ pMethodTable || .. object data .. ] + // ^-- the object reference points here + // + // The first DWORD of the method table class will have its high bit set if the + // method table has component size info stored somewhere. See member + // MethodTable:IsStringOrArray in src\vm\methodtable.h for full details. + // + // So in effect this method is the equivalent of + // return ((MethodTable*)(*obj))->IsStringOrArray(); + + Debug.Assert(obj != null); + return *(int*)GetObjectMethodTablePointer(obj) < 0; + } + + // Given an object reference, returns its MethodTable* as an IntPtr. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static IntPtr GetObjectMethodTablePointer(object obj) + { + Debug.Assert(obj != null); + + // We know that the first data field in any managed object is immediately after the + // method table pointer, so just back up one pointer and immediately deref. + // This is not ideal in terms of minimizing instruction count but is the best we can do at the moment. + + return Unsafe.Add(ref Unsafe.As(ref JitHelpers.GetPinningHelper(obj).m_data), -1); + + // The JIT currently implements this as: + // lea tmp, [rax + 8h] ; assume rax contains the object reference, tmp is type IntPtr& + // mov tmp, qword ptr [tmp - 8h] ; tmp now contains the MethodTable* pointer + // + // Ideally this would just be a single dereference: + // mov tmp, qword ptr [rax] ; rax = obj ref, tmp = MethodTable* pointer + } } } - diff --git a/src/coreclr/tests/CoreFX/CoreFX.issues.json b/src/coreclr/tests/CoreFX/CoreFX.issues.json index 059b61b..2f15282 100644 --- a/src/coreclr/tests/CoreFX/CoreFX.issues.json +++ b/src/coreclr/tests/CoreFX/CoreFX.issues.json @@ -486,6 +486,14 @@ { "name": "System.Buffers.Text.Tests.FormatterTests.TestFormatterDecimal", "reason": "https://github.com/dotnet/coreclr/pull/19775" + }, + { + "name": "System.SpanTests.MemoryMarshalTests.CreateFromPinnedArrayIntSliceRemainsPinned", + "reason": "https://github.com/dotnet/corefx/pull/32994" + }, + { + "name": "System.SpanTests.MemoryMarshalTests.CreateFromPinnedArrayIntReadOnlyMemorySliceRemainsPinned", + "reason": "https://github.com/dotnet/corefx/pull/32994" } ] } diff --git a/src/libraries/System.Private.CoreLib/src/System/Memory.cs b/src/libraries/System.Private.CoreLib/src/System/Memory.cs index e4596d3..033b806 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Memory.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Memory.cs @@ -24,20 +24,13 @@ namespace System // NOTE: With the current implementation, Memory and ReadOnlyMemory must have the same layout, // as code uses Unsafe.As to cast between them. - // The highest order bit of _index is used to discern whether _object is an array/string or an owned memory - // if (_index >> 31) == 1, object _object is an MemoryManager - // else, object _object is a T[] or a string. - // if (_length >> 31) == 1, _object is a pre-pinned array, so Pin() will not allocate a new GCHandle - // else, Pin() needs to allocate a new GCHandle to pin the object. - // It can only be a string if the Memory was created by - // using unsafe / marshaling code to reinterpret a ReadOnlyMemory wrapped around a string as - // a Memory. + // The highest order bit of _index is used to discern whether _object is a pre-pinned array. + // (_index < 0) => _object is a pre-pinned array, so Pin() will not allocate a new GCHandle + // (else) => Pin() needs to allocate a new GCHandle to pin the object. private readonly object _object; private readonly int _index; private readonly int _length; - - private const int RemoveFlagsBitMask = 0x7FFFFFFF; - + /// /// Creates a new memory over the entirety of the target array. /// @@ -137,8 +130,7 @@ namespace System ThrowHelper.ThrowArgumentOutOfRangeException(); _object = manager; - _index = (1 << 31); // Mark as MemoryManager type - // Before using _index, check if _index < 0, then 'and' it with RemoveFlagsBitMask + _index = 0; _length = length; } @@ -162,15 +154,18 @@ namespace System ThrowHelper.ThrowArgumentOutOfRangeException(); _object = manager; - _index = start | (1 << 31); // Mark as MemoryManager type - // Before using _index, check if _index < 0, then 'and' it with RemoveFlagsBitMask + _index = start; _length = length; } [MethodImpl(MethodImplOptions.AggressiveInlining)] internal Memory(object obj, int start, int length) { - // No validation performed; caller must provide any necessary validation. + // No validation performed in release builds; caller must provide any necessary validation. + + // 'obj is T[]' below also handles things like int[] <-> uint[] being convertible + Debug.Assert((obj == null) || (typeof(T) == typeof(char) && obj is string) || (obj is T[]) || (obj is MemoryManager)); + _object = obj; _index = start; _length = length; @@ -200,12 +195,12 @@ namespace System /// /// The number of items in the memory. /// - public int Length => _length & RemoveFlagsBitMask; + public int Length => _length; /// /// Returns true if Length is 0. /// - public bool IsEmpty => (_length & RemoveFlagsBitMask) == 0; + public bool IsEmpty => _length == 0; /// /// For , returns a new instance of string that represents the characters pointed to by the memory. @@ -215,9 +210,9 @@ namespace System { if (typeof(T) == typeof(char)) { - return (_object is string str) ? str.Substring(_index, _length & RemoveFlagsBitMask) : Span.ToString(); + return (_object is string str) ? str.Substring(_index, _length) : Span.ToString(); } - return string.Format("System.Memory<{0}>[{1}]", typeof(T).Name, _length & RemoveFlagsBitMask); + return string.Format("System.Memory<{0}>[{1}]", typeof(T).Name, _length); } /// @@ -230,16 +225,13 @@ namespace System [MethodImpl(MethodImplOptions.AggressiveInlining)] public Memory Slice(int start) { - // Used to maintain the high-bit which indicates whether the Memory has been pre-pinned or not. - int capturedLength = _length; - int actualLength = capturedLength & RemoveFlagsBitMask; - if ((uint)start > (uint)actualLength) + if ((uint)start > (uint)_length) { ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start); } - // It is expected for (capturedLength - start) to be negative if the memory is already pre-pinned. - return new Memory(_object, _index + start, capturedLength - start); + // It is expected for _index + start to be negative if the memory is already pre-pinned. + return new Memory(_object, _index + start, _length - start); } /// @@ -253,54 +245,103 @@ namespace System [MethodImpl(MethodImplOptions.AggressiveInlining)] public Memory Slice(int start, int length) { - // Used to maintain the high-bit which indicates whether the Memory has been pre-pinned or not. - int capturedLength = _length; - int actualLength = capturedLength & RemoveFlagsBitMask; #if BIT64 // See comment in Span.Slice for how this works. - if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)actualLength) + if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length) ThrowHelper.ThrowArgumentOutOfRangeException(); #else - if ((uint)start > (uint)actualLength || (uint)length > (uint)(actualLength - start)) + if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start)) ThrowHelper.ThrowArgumentOutOfRangeException(); #endif - // Set the high-bit to match the this._length high bit (1 for pre-pinned, 0 for unpinned). - return new Memory(_object, _index + start, length | (capturedLength & ~RemoveFlagsBitMask)); + // It is expected for _index + start to be negative if the memory is already pre-pinned. + return new Memory(_object, _index + start, length); } /// /// Returns a span from the memory. /// - public Span Span + public unsafe Span Span { [MethodImpl(MethodImplOptions.AggressiveInlining)] get { - if (_index < 0) - { - Debug.Assert(_length >= 0); - Debug.Assert(_object != null); - return ((MemoryManager)_object).GetSpan().Slice(_index & RemoveFlagsBitMask, _length); - } - else if (typeof(T) == typeof(char) && _object is string s) - { - Debug.Assert(_length >= 0); - // This is dangerous, returning a writable span for a string that should be immutable. - // However, we need to handle the case where a ReadOnlyMemory was created from a string - // and then cast to a Memory. Such a cast can only be done with unsafe or marshaling code, - // in which case that's the dangerous operation performed by the dev, and we're just following - // suit here to make it work as best as possible. - return new Span(ref Unsafe.As(ref s.GetRawStringData()), s.Length).Slice(_index, _length); - } - else if (_object != null) - { - return new Span((T[])_object, _index, _length & RemoveFlagsBitMask); - } - else + // This property getter has special support for returning a mutable Span that wraps + // an immutable String instance. This is obviously a dangerous feature and breaks type safety. + // However, we need to handle the case where a ReadOnlyMemory was created from a string + // and then cast to a Memory. Such a cast can only be done with unsafe or marshaling code, + // in which case that's the dangerous operation performed by the dev, and we're just following + // suit here to make it work as best as possible. + + ref T refToReturn = ref Unsafe.AsRef(null); + int lengthOfUnderlyingSpan = 0; + + // Copy this field into a local so that it can't change out from under us mid-operation. + + object tmpObject = _object; + if (tmpObject != null) { - return default; + if (typeof(T) == typeof(char) && tmpObject.GetType() == typeof(string)) + { + // Special-case string since it's the most common for ROM. + + refToReturn = ref Unsafe.As(ref Unsafe.As(tmpObject).GetRawStringData()); + lengthOfUnderlyingSpan = Unsafe.As(tmpObject).Length; + } + else if (RuntimeHelpers.ObjectHasComponentSize(tmpObject)) + { + // We know the object is not null, it's not a string, and it is variable-length. The only + // remaining option is for it to be a T[] (or a U[] which is blittable to T[], like int[] + // and uint[]). Otherwise somebody used private reflection to set this field, and we're not + // too worried about type safety violations at this point. + + // 'tmpObject is T[]' below also handles things like int[] <-> uint[] being convertible + Debug.Assert(tmpObject is T[]); + + refToReturn = ref Unsafe.As(ref Unsafe.As(tmpObject).GetRawSzArrayData()); + lengthOfUnderlyingSpan = Unsafe.As(tmpObject).Length; + } + else + { + // We know the object is not null, and it's not variable-length, so it must be a MemoryManager. + // Otherwise somebody used private reflection to set this field, and we're not too worried about + // type safety violations at that point. Note that it can't be a MemoryManager, even if U and + // T are blittable (e.g., MemoryManager to MemoryManager), since there exists no + // constructor or other public API which would allow such a conversion. + + Debug.Assert(tmpObject is MemoryManager); + Span memoryManagerSpan = Unsafe.As>(tmpObject).GetSpan(); + refToReturn = ref MemoryMarshal.GetReference(memoryManagerSpan); + lengthOfUnderlyingSpan = memoryManagerSpan.Length; + } + + // If the Memory or ReadOnlyMemory instance is torn, this property getter has undefined behavior. + // We try to detect this condition and throw an exception, but it's possible that a torn struct might + // appear to us to be valid, and we'll return an undesired span. Such a span is always guaranteed at + // least to be in-bounds when compared with the original Memory instance, so using the span won't + // AV the process. + + int desiredStartIndex = _index & ReadOnlyMemory.RemoveFlagsBitMask; + int desiredLength = _length; + +#if BIT64 + // See comment in Span.Slice for how this works. + if ((ulong)(uint)desiredStartIndex + (ulong)(uint)desiredLength > (ulong)(uint)lengthOfUnderlyingSpan) + { + ThrowHelper.ThrowArgumentOutOfRangeException(); + } +#else + if ((uint)desiredStartIndex > (uint)lengthOfUnderlyingSpan || (uint)desiredLength > (uint)(lengthOfUnderlyingSpan - desiredStartIndex)) + { + ThrowHelper.ThrowArgumentOutOfRangeException(); + } +#endif + + refToReturn = ref Unsafe.Add(ref refToReturn, desiredStartIndex); + lengthOfUnderlyingSpan = desiredLength; } + + return new Span(ref refToReturn, lengthOfUnderlyingSpan); } } @@ -337,37 +378,51 @@ namespace System /// public unsafe MemoryHandle Pin() { - if (_index < 0) - { - Debug.Assert(_object != null); - return ((MemoryManager)_object).Pin((_index & RemoveFlagsBitMask)); - } - else if (typeof(T) == typeof(char) && _object is string s) + // Just like the Span property getter, we have special support for a mutable Memory + // that wraps an immutable String instance. This might happen if a caller creates an + // immutable ROM wrapping a String, then uses Unsafe.As to create a mutable M. + // This needs to work, however, so that code that uses a single Memory field to store either + // a readable ReadOnlyMemory or a writable Memory can still be pinned and + // used for interop purposes. + + // It's possible that the below logic could result in an AV if the struct + // is torn. This is ok since the caller is expecting to use raw pointers, + // and we're not required to keep this as safe as the other Span-based APIs. + + object tmpObject = _object; + if (tmpObject != null) { - // This case can only happen if a ReadOnlyMemory was created around a string - // and then that was cast to a Memory using unsafe / marshaling code. This needs - // to work, however, so that code that uses a single Memory field to store either - // a readable ReadOnlyMemory or a writable Memory can still be pinned and - // used for interop purposes. - GCHandle handle = GCHandle.Alloc(s, GCHandleType.Pinned); - void* pointer = Unsafe.Add(Unsafe.AsPointer(ref s.GetRawStringData()), _index); - return new MemoryHandle(pointer, handle); - } - else if (_object is T[] array) - { - // Array is already pre-pinned - if (_length < 0) + if (typeof(T) == typeof(char) && tmpObject is string s) + { + GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned); + ref char stringData = ref Unsafe.Add(ref s.GetRawStringData(), _index); + return new MemoryHandle(Unsafe.AsPointer(ref stringData), handle); + } + else if (RuntimeHelpers.ObjectHasComponentSize(tmpObject)) { - void* pointer = Unsafe.Add(Unsafe.AsPointer(ref array.GetRawSzArrayData()), _index); - return new MemoryHandle(pointer); + // 'tmpObject is T[]' below also handles things like int[] <-> uint[] being convertible + Debug.Assert(tmpObject is T[]); + + // Array is already pre-pinned + if (_index < 0) + { + void* pointer = Unsafe.Add(Unsafe.AsPointer(ref Unsafe.As(tmpObject).GetRawSzArrayData()), _index & ReadOnlyMemory.RemoveFlagsBitMask); + return new MemoryHandle(pointer); + } + else + { + GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned); + void* pointer = Unsafe.Add(Unsafe.AsPointer(ref Unsafe.As(tmpObject).GetRawSzArrayData()), _index); + return new MemoryHandle(pointer, handle); + } } else { - GCHandle handle = GCHandle.Alloc(array, GCHandleType.Pinned); - void* pointer = Unsafe.Add(Unsafe.AsPointer(ref array.GetRawSzArrayData()), _index); - return new MemoryHandle(pointer, handle); + Debug.Assert(tmpObject is MemoryManager); + return Unsafe.As>(tmpObject).Pin(_index); } } + return default; } @@ -417,18 +472,9 @@ namespace System [EditorBrowsable(EditorBrowsableState.Never)] public override int GetHashCode() { - return _object != null ? CombineHashCodes(_object.GetHashCode(), _index.GetHashCode(), _length.GetHashCode()) : 0; - } - - private static int CombineHashCodes(int left, int right) - { - return ((left << 5) + left) ^ right; + // We use RuntimeHelpers.GetHashCode instead of Object.GetHashCode because the hash + // code is based on object identity and referential equality, not deep equality (as common with string). + return (_object != null) ? HashCode.Combine(RuntimeHelpers.GetHashCode(_object), _index, _length) : 0; } - - private static int CombineHashCodes(int h1, int h2, int h3) - { - return CombineHashCodes(CombineHashCodes(h1, h2), h3); - } - } } diff --git a/src/libraries/System.Private.CoreLib/src/System/ReadOnlyMemory.cs b/src/libraries/System.Private.CoreLib/src/System/ReadOnlyMemory.cs index 7a332c1..8fd659a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/ReadOnlyMemory.cs +++ b/src/libraries/System.Private.CoreLib/src/System/ReadOnlyMemory.cs @@ -24,11 +24,9 @@ namespace System // NOTE: With the current implementation, Memory and ReadOnlyMemory must have the same layout, // as code uses Unsafe.As to cast between them. - // The highest order bit of _index is used to discern whether _object is an array/string or an owned memory - // if (_index >> 31) == 1, _object is an MemoryManager - // else, _object is a T[] or string. - // if (_length >> 31) == 1, _object is a pre-pinned array, so Pin() will not allocate a new GCHandle - // else, Pin() needs to allocate a new GCHandle to pin the object. + // The highest order bit of _index is used to discern whether _object is a pre-pinned array. + // (_index < 0) => _object is a pre-pinned array, so Pin() will not allocate a new GCHandle + // (else) => Pin() needs to allocate a new GCHandle to pin the object. private readonly object _object; private readonly int _index; private readonly int _length; @@ -98,7 +96,11 @@ namespace System [MethodImpl(MethodImplOptions.AggressiveInlining)] internal ReadOnlyMemory(object obj, int start, int length) { - // No validation performed; caller must provide any necessary validation. + // No validation performed in release builds; caller must provide any necessary validation. + + // 'obj is T[]' below also handles things like int[] <-> uint[] being convertible + Debug.Assert((obj == null) || (typeof(T) == typeof(char) && obj is string) || (obj is T[]) || (obj is MemoryManager)); + _object = obj; _index = start; _length = length; @@ -122,12 +124,12 @@ namespace System /// /// The number of items in the memory. /// - public int Length => _length & RemoveFlagsBitMask; + public int Length => _length; /// /// Returns true if Length is 0. /// - public bool IsEmpty => (_length & RemoveFlagsBitMask) == 0; + public bool IsEmpty => _length == 0; /// /// For , returns a new instance of string that represents the characters pointed to by the memory. @@ -137,9 +139,9 @@ namespace System { if (typeof(T) == typeof(char)) { - return (_object is string str) ? str.Substring(_index, _length & RemoveFlagsBitMask) : Span.ToString(); + return (_object is string str) ? str.Substring(_index, _length) : Span.ToString(); } - return string.Format("System.ReadOnlyMemory<{0}>[{1}]", typeof(T).Name, _length & RemoveFlagsBitMask); + return string.Format("System.ReadOnlyMemory<{0}>[{1}]", typeof(T).Name, _length); } /// @@ -152,16 +154,13 @@ namespace System [MethodImpl(MethodImplOptions.AggressiveInlining)] public ReadOnlyMemory Slice(int start) { - // Used to maintain the high-bit which indicates whether the Memory has been pre-pinned or not. - int capturedLength = _length; - int actualLength = capturedLength & RemoveFlagsBitMask; - if ((uint)start > (uint)actualLength) + if ((uint)start > (uint)_length) { ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start); } - // It is expected for (capturedLength - start) to be negative if the memory is already pre-pinned. - return new ReadOnlyMemory(_object, _index + start, capturedLength - start); + // It is expected for _index + start to be negative if the memory is already pre-pinned. + return new ReadOnlyMemory(_object, _index + start, _length - start); } /// @@ -175,49 +174,96 @@ namespace System [MethodImpl(MethodImplOptions.AggressiveInlining)] public ReadOnlyMemory Slice(int start, int length) { - // Used to maintain the high-bit which indicates whether the Memory has been pre-pinned or not. - int capturedLength = _length; - int actualLength = _length & RemoveFlagsBitMask; #if BIT64 // See comment in Span.Slice for how this works. - if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)actualLength) + if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start); #else - if ((uint)start > (uint)actualLength || (uint)length > (uint)(actualLength - start)) + if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start)) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start); #endif - // Set the high-bit to match the this._length high bit (1 for pre-pinned, 0 for unpinned). - return new ReadOnlyMemory(_object, _index + start, length | (capturedLength & ~RemoveFlagsBitMask)); + // It is expected for _index + start to be negative if the memory is already pre-pinned. + return new ReadOnlyMemory(_object, _index + start, length); } /// /// Returns a span from the memory. /// - public ReadOnlySpan Span + public unsafe ReadOnlySpan Span { [MethodImpl(MethodImplOptions.AggressiveInlining)] get { - if (_index < 0) - { - Debug.Assert(_length >= 0); - Debug.Assert(_object != null); - return ((MemoryManager)_object).GetSpan().Slice(_index & RemoveFlagsBitMask, _length); - } - else if (typeof(T) == typeof(char) && _object is string s) - { - Debug.Assert(_length >= 0); - return new ReadOnlySpan(ref Unsafe.As(ref s.GetRawStringData()), s.Length).Slice(_index, _length); - } - else if (_object != null) - { - return new ReadOnlySpan((T[])_object, _index, _length & RemoveFlagsBitMask); - } - else + ref T refToReturn = ref Unsafe.AsRef(null); + int lengthOfUnderlyingSpan = 0; + + // Copy this field into a local so that it can't change out from under us mid-operation. + + object tmpObject = _object; + if (tmpObject != null) { - return default; + if (typeof(T) == typeof(char) && tmpObject.GetType() == typeof(string)) + { + // Special-case string since it's the most common for ROM. + + refToReturn = ref Unsafe.As(ref Unsafe.As(tmpObject).GetRawStringData()); + lengthOfUnderlyingSpan = Unsafe.As(tmpObject).Length; + } + else if (RuntimeHelpers.ObjectHasComponentSize(tmpObject)) + { + // We know the object is not null, it's not a string, and it is variable-length. The only + // remaining option is for it to be a T[] (or a U[] which is blittable to T[], like int[] + // and uint[]). Otherwise somebody used private reflection to set this field, and we're not + // too worried about type safety violations at this point. + + // 'tmpObject is T[]' below also handles things like int[] <-> uint[] being convertible + Debug.Assert(tmpObject is T[]); + + refToReturn = ref Unsafe.As(ref Unsafe.As(tmpObject).GetRawSzArrayData()); + lengthOfUnderlyingSpan = Unsafe.As(tmpObject).Length; + } + else + { + // We know the object is not null, and it's not variable-length, so it must be a MemoryManager. + // Otherwise somebody used private reflection to set this field, and we're not too worried about + // type safety violations at that point. Note that it can't be a MemoryManager, even if U and + // T are blittable (e.g., MemoryManager to MemoryManager), since there exists no + // constructor or other public API which would allow such a conversion. + + Debug.Assert(tmpObject is MemoryManager); + Span memoryManagerSpan = Unsafe.As>(tmpObject).GetSpan(); + refToReturn = ref MemoryMarshal.GetReference(memoryManagerSpan); + lengthOfUnderlyingSpan = memoryManagerSpan.Length; + } + + // If the Memory or ReadOnlyMemory instance is torn, this property getter has undefined behavior. + // We try to detect this condition and throw an exception, but it's possible that a torn struct might + // appear to us to be valid, and we'll return an undesired span. Such a span is always guaranteed at + // least to be in-bounds when compared with the original Memory instance, so using the span won't + // AV the process. + + int desiredStartIndex = _index & RemoveFlagsBitMask; + int desiredLength = _length; + +#if BIT64 + // See comment in Span.Slice for how this works. + if ((ulong)(uint)desiredStartIndex + (ulong)(uint)desiredLength > (ulong)(uint)lengthOfUnderlyingSpan) + { + ThrowHelper.ThrowArgumentOutOfRangeException(); + } +#else + if ((uint)desiredStartIndex > (uint)lengthOfUnderlyingSpan || (uint)desiredLength > (uint)(lengthOfUnderlyingSpan - desiredStartIndex)) + { + ThrowHelper.ThrowArgumentOutOfRangeException(); + } +#endif + + refToReturn = ref Unsafe.Add(ref refToReturn, desiredStartIndex); + lengthOfUnderlyingSpan = desiredLength; } + + return new ReadOnlySpan(ref refToReturn, lengthOfUnderlyingSpan); } } @@ -254,32 +300,44 @@ namespace System /// public unsafe MemoryHandle Pin() { - if (_index < 0) - { - Debug.Assert(_object != null); - return ((MemoryManager)_object).Pin((_index & RemoveFlagsBitMask)); - } - else if (typeof(T) == typeof(char) && _object is string s) - { - GCHandle handle = GCHandle.Alloc(s, GCHandleType.Pinned); - void* pointer = Unsafe.Add(Unsafe.AsPointer(ref s.GetRawStringData()), _index); - return new MemoryHandle(pointer, handle); - } - else if (_object is T[] array) + // It's possible that the below logic could result in an AV if the struct + // is torn. This is ok since the caller is expecting to use raw pointers, + // and we're not required to keep this as safe as the other Span-based APIs. + + object tmpObject = _object; + if (tmpObject != null) { - // Array is already pre-pinned - if (_length < 0) + if (typeof(T) == typeof(char) && tmpObject is string s) { - void* pointer = Unsafe.Add(Unsafe.AsPointer(ref array.GetRawSzArrayData()), _index); - return new MemoryHandle(pointer); + GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned); + ref char stringData = ref Unsafe.Add(ref s.GetRawStringData(), _index); + return new MemoryHandle(Unsafe.AsPointer(ref stringData), handle); + } + else if (RuntimeHelpers.ObjectHasComponentSize(tmpObject)) + { + // 'tmpObject is T[]' below also handles things like int[] <-> uint[] being convertible + Debug.Assert(tmpObject is T[]); + + // Array is already pre-pinned + if (_index < 0) + { + void* pointer = Unsafe.Add(Unsafe.AsPointer(ref Unsafe.As(tmpObject).GetRawSzArrayData()), _index & RemoveFlagsBitMask); + return new MemoryHandle(pointer); + } + else + { + GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned); + void* pointer = Unsafe.Add(Unsafe.AsPointer(ref Unsafe.As(tmpObject).GetRawSzArrayData()), _index); + return new MemoryHandle(pointer, handle); + } } else { - GCHandle handle = GCHandle.Alloc(array, GCHandleType.Pinned); - void* pointer = Unsafe.Add(Unsafe.AsPointer(ref array.GetRawSzArrayData()), _index); - return new MemoryHandle(pointer, handle); + Debug.Assert(tmpObject is MemoryManager); + return Unsafe.As>(tmpObject).Pin(_index); } } + return default; } @@ -324,19 +382,11 @@ namespace System [EditorBrowsable(EditorBrowsableState.Never)] public override int GetHashCode() { - return _object != null ? CombineHashCodes(_object.GetHashCode(), _index.GetHashCode(), _length.GetHashCode()) : 0; - } - - private static int CombineHashCodes(int left, int right) - { - return ((left << 5) + left) ^ right; + // We use RuntimeHelpers.GetHashCode instead of Object.GetHashCode because the hash + // code is based on object identity and referential equality, not deep equality (as common with string). + return (_object != null) ? HashCode.Combine(RuntimeHelpers.GetHashCode(_object), _index, _length) : 0; } - - private static int CombineHashCodes(int h1, int h2, int h3) - { - return CombineHashCodes(CombineHashCodes(h1, h2), h3); - } - + /// Gets the state of the memory as individual fields. /// The offset. /// The count. diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs index fcc9c4f..d2d94fd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs @@ -24,27 +24,50 @@ namespace System.Runtime.InteropServices public static bool TryGetArray(ReadOnlyMemory memory, out ArraySegment segment) { object obj = memory.GetObjectStartLength(out int index, out int length); - if (index < 0) + + // As an optimization, we skip the "is string?" check below if typeof(T) is not char, + // as Memory / ROM can't possibly contain a string instance in this case. + + if (obj != null && (typeof(T) != typeof(char) || obj.GetType() != typeof(string))) { - Debug.Assert(length >= 0); - if (((MemoryManager)obj).TryGetArray(out ArraySegment arraySegment)) + if (RuntimeHelpers.ObjectHasComponentSize(obj)) { - segment = new ArraySegment(arraySegment.Array, arraySegment.Offset + (index & ReadOnlyMemory.RemoveFlagsBitMask), length); + // The object has a component size, which means it's variable-length, but we already + // checked above that it's not a String. The only remaining option is that it's a T[] + // or a U[] which is blittable to a T[] (e.g., int[] and uint[]). + + // The array may be prepinned, so remove the high bit from the start index in the line below. + // The ArraySegment ctor will perform bounds checking on index & length. + + segment = new ArraySegment(Unsafe.As(obj), index & ReadOnlyMemory.RemoveFlagsBitMask, length); return true; } + else + { + // The object isn't null, and it's not variable-length, so the only remaining option + // is MemoryManager. The ArraySegment ctor will perform bounds checking on index & length. + + Debug.Assert(obj is MemoryManager); + if (Unsafe.As>(obj).TryGetArray(out ArraySegment tempArraySegment)) + { + segment = new ArraySegment(tempArraySegment.Array, tempArraySegment.Offset + index, length); + return true; + } + } } - else if (obj is T[] arr) - { - segment = new ArraySegment(arr, index, length & ReadOnlyMemory.RemoveFlagsBitMask); - return true; - } - if ((length & ReadOnlyMemory.RemoveFlagsBitMask) == 0) + // If we got to this point, the object is null, or it's a string, or it's a MemoryManager + // which isn't backed by an array. We'll quickly homogenize all zero-length Memory instances + // to an empty array for the purposes of reporting back to our caller. + + if (length == 0) { segment = ArraySegment.Empty; return true; } + // Otherwise, there's *some* data, but it's not convertible to T[]. + segment = default; return false; } @@ -82,7 +105,6 @@ namespace System.Runtime.InteropServices { TManager localManager; // Use register for null comparison rather than byref manager = localManager = memory.GetObjectStartLength(out start, out length) as TManager; - start &= ReadOnlyMemory.RemoveFlagsBitMask; Debug.Assert(length >= 0); @@ -284,8 +306,8 @@ namespace System.Runtime.InteropServices if ((uint)start > (uint)array.Length || (uint)length > (uint)(array.Length - start)) ThrowHelper.ThrowArgumentOutOfRangeException(); - // Before using _length, check if _length < 0, then 'and' it with RemoveFlagsBitMask - return new Memory((object)array, start, length | (1 << 31)); + // Before using _index, check if _index < 0, then 'and' it with RemoveFlagsBitMask + return new Memory((object)array, start | (1 << 31), length); } } } -- 2.7.4