Improve performance of Memory<T>.Span property getter (#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.

5 files changed, 345 insertions, 178 deletions

diff --git a/src/System.Private.CoreLib/shared/System/Memory.cs b/src/System.Private.CoreLib/shared/System/Memory.cs
index e4596d3119..033b806c20 100644
--- a/src/System.Private.CoreLib/shared/System/Memory.cs
+++ b/src/System.Private.CoreLib/shared/System/Memory.cs
@@ -24,20 +24,13 @@ namespace System
         // NOTE: With the current implementation, Memory<T> and ReadOnlyMemory<T> 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<T>
-        // 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<T> was created by
-        // using unsafe / marshaling code to reinterpret a ReadOnlyMemory<char> wrapped around a string as
-        // a Memory<T>.
+        // 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;
-
+        
         /// <summary>
         /// Creates a new memory over the entirety of the target array.
         /// </summary>
@@ -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<T>));
+
             _object = obj;
             _index = start;
             _length = length;
@@ -200,12 +195,12 @@ namespace System
         /// <summary>
         /// The number of items in the memory.
         /// </summary>
-        public int Length => _length & RemoveFlagsBitMask;
+        public int Length => _length;
 
         /// <summary>
         /// Returns true if Length is 0.
         /// </summary>
-        public bool IsEmpty => (_length & RemoveFlagsBitMask) == 0;
+        public bool IsEmpty => _length == 0;
 
         /// <summary>
         /// For <see cref="Memory{Char}"/>, 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);
         }
 
         /// <summary>
@@ -230,16 +225,13 @@ namespace System
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public Memory<T> 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<T>(_object, _index + start, capturedLength - start);
+            // It is expected for _index + start to be negative if the memory is already pre-pinned.
+            return new Memory<T>(_object, _index + start, _length - start);
         }
 
         /// <summary>
@@ -253,54 +245,103 @@ namespace System
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public Memory<T> 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<T>.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<T>(_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<T>(_object, _index + start, length);
         }
 
         /// <summary>
         /// Returns a span from the memory.
         /// </summary>
-        public Span<T> Span
+        public unsafe Span<T> Span
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
             get
             {
-                if (_index < 0)
-                {
-                    Debug.Assert(_length >= 0);
-                    Debug.Assert(_object != null);
-                    return ((MemoryManager<T>)_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<char> was created from a string
-                    // and then cast to a Memory<T>. 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<T>(ref Unsafe.As<char, T>(ref s.GetRawStringData()), s.Length).Slice(_index, _length);
-                }
-                else if (_object != null)
-                {
-                    return new Span<T>((T[])_object, _index, _length & RemoveFlagsBitMask);
-                }
-                else
+                // This property getter has special support for returning a mutable Span<char> 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<char> was created from a string
+                // and then cast to a Memory<T>. 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<T>(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<char>.
+
+                        refToReturn = ref Unsafe.As<char, T>(ref Unsafe.As<string>(tmpObject).GetRawStringData());
+                        lengthOfUnderlyingSpan = Unsafe.As<string>(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<byte, T>(ref Unsafe.As<T[]>(tmpObject).GetRawSzArrayData());
+                        lengthOfUnderlyingSpan = Unsafe.As<T[]>(tmpObject).Length;
+                    }
+                    else
+                    {
+                        // We know the object is not null, and it's not variable-length, so it must be a MemoryManager<T>.
+                        // 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<U>, even if U and
+                        // T are blittable (e.g., MemoryManager<int> to MemoryManager<uint>), since there exists no
+                        // constructor or other public API which would allow such a conversion.
+
+                        Debug.Assert(tmpObject is MemoryManager<T>);
+                        Span<T> memoryManagerSpan = Unsafe.As<MemoryManager<T>>(tmpObject).GetSpan();
+                        refToReturn = ref MemoryMarshal.GetReference(memoryManagerSpan);
+                        lengthOfUnderlyingSpan = memoryManagerSpan.Length;
+                    }
+
+                    // If the Memory<T> or ReadOnlyMemory<T> 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<T> instance, so using the span won't
+                    // AV the process.
+
+                    int desiredStartIndex = _index & ReadOnlyMemory<T>.RemoveFlagsBitMask;
+                    int desiredLength = _length;
+
+#if BIT64
+                    // See comment in Span<T>.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<T>(ref refToReturn, lengthOfUnderlyingSpan);
             }
         }
 
@@ -337,37 +378,51 @@ namespace System
         /// </summary>
         public unsafe MemoryHandle Pin()
         {
-            if (_index < 0)
-            {
-                Debug.Assert(_object != null);
-                return ((MemoryManager<T>)_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<char>
+            // that wraps an immutable String instance. This might happen if a caller creates an
+            // immutable ROM<char> wrapping a String, then uses Unsafe.As to create a mutable M<char>.
+            // This needs to work, however, so that code that uses a single Memory<char> field to store either
+            // a readable ReadOnlyMemory<char> or a writable Memory<char> 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<char> was created around a string
-                // and then that was cast to a Memory<char> using unsafe / marshaling code.  This needs
-                // to work, however, so that code that uses a single Memory<char> field to store either
-                // a readable ReadOnlyMemory<char> or a writable Memory<char> can still be pinned and
-                // used for interop purposes.
-                GCHandle handle = GCHandle.Alloc(s, GCHandleType.Pinned);
-                void* pointer = Unsafe.Add<T>(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<T>(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<T>(Unsafe.AsPointer(ref Unsafe.As<T[]>(tmpObject).GetRawSzArrayData()), _index & ReadOnlyMemory<T>.RemoveFlagsBitMask);
+                        return new MemoryHandle(pointer);
+                    }
+                    else
+                    {
+                        GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned);
+                        void* pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref Unsafe.As<T[]>(tmpObject).GetRawSzArrayData()), _index);
+                        return new MemoryHandle(pointer, handle);
+                    }
                 }
                 else
                 {
-                    GCHandle handle = GCHandle.Alloc(array, GCHandleType.Pinned);
-                    void* pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref array.GetRawSzArrayData()), _index);
-                    return new MemoryHandle(pointer, handle);
+                    Debug.Assert(tmpObject is MemoryManager<T>);
+                    return Unsafe.As<MemoryManager<T>>(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/System.Private.CoreLib/shared/System/ReadOnlyMemory.cs b/src/System.Private.CoreLib/shared/System/ReadOnlyMemory.cs
index 7a332c1fa3..8fd659aeaa 100644
--- a/src/System.Private.CoreLib/shared/System/ReadOnlyMemory.cs
+++ b/src/System.Private.CoreLib/shared/System/ReadOnlyMemory.cs
@@ -24,11 +24,9 @@ namespace System
         // NOTE: With the current implementation, Memory<T> and ReadOnlyMemory<T> 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<T>
-        // 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<T>));
+
             _object = obj;
             _index = start;
             _length = length;
@@ -122,12 +124,12 @@ namespace System
         /// <summary>
         /// The number of items in the memory.
         /// </summary>
-        public int Length => _length & RemoveFlagsBitMask;
+        public int Length => _length;
 
         /// <summary>
         /// Returns true if Length is 0.
         /// </summary>
-        public bool IsEmpty => (_length & RemoveFlagsBitMask) == 0;
+        public bool IsEmpty => _length == 0;
 
         /// <summary>
         /// For <see cref="ReadOnlyMemory{Char}"/>, 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);
         }
 
         /// <summary>
@@ -152,16 +154,13 @@ namespace System
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public ReadOnlyMemory<T> 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<T>(_object, _index + start, capturedLength - start);
+            // It is expected for _index + start to be negative if the memory is already pre-pinned.
+            return new ReadOnlyMemory<T>(_object, _index + start, _length - start);
         }
 
         /// <summary>
@@ -175,49 +174,96 @@ namespace System
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public ReadOnlyMemory<T> 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<T>.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<T>(_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<T>(_object, _index + start, length);
         }
 
         /// <summary>
         /// Returns a span from the memory.
         /// </summary>
-        public ReadOnlySpan<T> Span
+        public unsafe ReadOnlySpan<T> Span
         {
             [MethodImpl(MethodImplOptions.AggressiveInlining)]
             get
             {
-                if (_index < 0)
-                {
-                    Debug.Assert(_length >= 0);
-                    Debug.Assert(_object != null);
-                    return ((MemoryManager<T>)_object).GetSpan().Slice(_index & RemoveFlagsBitMask, _length);
-                }
-                else if (typeof(T) == typeof(char) && _object is string s)
-                {
-                    Debug.Assert(_length >= 0);
-                    return new ReadOnlySpan<T>(ref Unsafe.As<char, T>(ref s.GetRawStringData()), s.Length).Slice(_index, _length);
-                }
-                else if (_object != null)
-                {
-                    return new ReadOnlySpan<T>((T[])_object, _index, _length & RemoveFlagsBitMask);
-                }
-                else
+                ref T refToReturn = ref Unsafe.AsRef<T>(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<char>.
+
+                        refToReturn = ref Unsafe.As<char, T>(ref Unsafe.As<string>(tmpObject).GetRawStringData());
+                        lengthOfUnderlyingSpan = Unsafe.As<string>(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<byte, T>(ref Unsafe.As<T[]>(tmpObject).GetRawSzArrayData());
+                        lengthOfUnderlyingSpan = Unsafe.As<T[]>(tmpObject).Length;
+                    }
+                    else
+                    {
+                        // We know the object is not null, and it's not variable-length, so it must be a MemoryManager<T>.
+                        // 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<U>, even if U and
+                        // T are blittable (e.g., MemoryManager<int> to MemoryManager<uint>), since there exists no
+                        // constructor or other public API which would allow such a conversion.
+
+                        Debug.Assert(tmpObject is MemoryManager<T>);
+                        Span<T> memoryManagerSpan = Unsafe.As<MemoryManager<T>>(tmpObject).GetSpan();
+                        refToReturn = ref MemoryMarshal.GetReference(memoryManagerSpan);
+                        lengthOfUnderlyingSpan = memoryManagerSpan.Length;
+                    }
+
+                    // If the Memory<T> or ReadOnlyMemory<T> 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<T> instance, so using the span won't
+                    // AV the process.
+
+                    int desiredStartIndex = _index & RemoveFlagsBitMask;
+                    int desiredLength = _length;
+
+#if BIT64
+                    // See comment in Span<T>.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<T>(ref refToReturn, lengthOfUnderlyingSpan);
             }
         }
 
@@ -254,32 +300,44 @@ namespace System
         /// </summary>
         public unsafe MemoryHandle Pin()
         {
-            if (_index < 0)
-            {
-                Debug.Assert(_object != null);
-                return ((MemoryManager<T>)_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<T>(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<T>(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<T>(Unsafe.AsPointer(ref Unsafe.As<T[]>(tmpObject).GetRawSzArrayData()), _index & RemoveFlagsBitMask);
+                        return new MemoryHandle(pointer);
+                    }
+                    else
+                    {
+                        GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned);
+                        void* pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref Unsafe.As<T[]>(tmpObject).GetRawSzArrayData()), _index);
+                        return new MemoryHandle(pointer, handle);
+                    }
                 }
                 else
                 {
-                    GCHandle handle = GCHandle.Alloc(array, GCHandleType.Pinned);
-                    void* pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref array.GetRawSzArrayData()), _index);
-                    return new MemoryHandle(pointer, handle);
+                    Debug.Assert(tmpObject is MemoryManager<T>);
+                    return Unsafe.As<MemoryManager<T>>(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);
-        }
-
+        
         /// <summary>Gets the state of the memory as individual fields.</summary>
         /// <param name="start">The offset.</param>
         /// <param name="length">The count.</param>
diff --git a/src/System.Private.CoreLib/shared/System/Runtime/InteropServices/MemoryMarshal.cs b/src/System.Private.CoreLib/shared/System/Runtime/InteropServices/MemoryMarshal.cs
index fcc9c4f41a..d2d94fdfef 100644
--- a/src/System.Private.CoreLib/shared/System/Runtime/InteropServices/MemoryMarshal.cs
+++ b/src/System.Private.CoreLib/shared/System/Runtime/InteropServices/MemoryMarshal.cs
@@ -24,27 +24,50 @@ namespace System.Runtime.InteropServices
         public static bool TryGetArray<T>(ReadOnlyMemory<T> memory, out ArraySegment<T> 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<T> / ROM<T> 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<T>)obj).TryGetArray(out ArraySegment<T> arraySegment))
+                if (RuntimeHelpers.ObjectHasComponentSize(obj))
                 {
-                    segment = new ArraySegment<T>(arraySegment.Array, arraySegment.Offset + (index & ReadOnlyMemory<T>.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<T> ctor will perform bounds checking on index & length.
+
+                    segment = new ArraySegment<T>(Unsafe.As<T[]>(obj), index & ReadOnlyMemory<T>.RemoveFlagsBitMask, length);
                     return true;
                 }
+                else
+                {
+                    // The object isn't null, and it's not variable-length, so the only remaining option
+                    // is MemoryManager<T>. The ArraySegment<T> ctor will perform bounds checking on index & length.
+
+                    Debug.Assert(obj is MemoryManager<T>);
+                    if (Unsafe.As<MemoryManager<T>>(obj).TryGetArray(out ArraySegment<T> tempArraySegment))
+                    {
+                        segment = new ArraySegment<T>(tempArraySegment.Array, tempArraySegment.Offset + index, length);
+                        return true;
+                    }
+                }
             }
-            else if (obj is T[] arr)
-            {
-                segment = new ArraySegment<T>(arr, index, length & ReadOnlyMemory<T>.RemoveFlagsBitMask);
-                return true;
-            }
 
-            if ((length & ReadOnlyMemory<T>.RemoveFlagsBitMask) == 0)
+            // If we got to this point, the object is null, or it's a string, or it's a MemoryManager<T>
+            // which isn't backed by an array. We'll quickly homogenize all zero-length Memory<T> instances
+            // to an empty array for the purposes of reporting back to our caller.
+
+            if (length == 0)
             {
                 segment = ArraySegment<T>.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<T>.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<T>((object)array, start, length | (1 << 31));
+            // Before using _index, check if _index < 0, then 'and' it with RemoveFlagsBitMask
+            return new Memory<T>((object)array, start | (1 << 31), length);
         }
     }
 }
diff --git a/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs b/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs
index 53b2848084..93490d933a 100644
--- a/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs
+++ b/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<byte, IntPtr>(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/tests/CoreFX/CoreFX.issues.json b/tests/CoreFX/CoreFX.issues.json
index 059b61b285..2f15282a7f 100644
--- a/tests/CoreFX/CoreFX.issues.json
+++ b/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"
                 }
             ]
         }