Add GetReference and TryGetArray to MemoryMarshal#15417
Conversation
| public static Span<T> Empty => default(Span<T>); | ||
|
|
||
| // This exposes the internal representation for Span-related apis use only. | ||
| internal ByReference<T> Pointer => _pointer; |
There was a problem hiding this comment.
This should return ref T. ByReference<T> should be used only for the fields.
There was a problem hiding this comment.
Why is that?
Wouldn't this make the implementation, essentially a duplicate, but internal, version of DangerousGetPinnableReference?
i.e.
internal ref T Reference => ref _pointer.Value;Do we then just return Reference from MemoryMarshal.GetReference()?
| public static Memory<T> AsMemory<T>(ReadOnlyMemory<T> readOnlyMemory) => | ||
| Unsafe.As<ReadOnlyMemory<T>, Memory<T>>(ref readOnlyMemory); | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Why is the AggresiveInlining necessary?
I know that you got into habit of marking everything with AggresiveInlining. I do not think it is a good idea to do blindly - it should be done only when you have a clear data that it helps.
There was a problem hiding this comment.
For this instance, I wanted to remain consistent with DangerousGetPinnableReference which already had it, presumable for good reason. I will try to run some experiments.
There was a problem hiding this comment.
Inlining in almost all cases made no difference.
The only case inlining helps is if the implementation is using in and the Reference property.
| Unsafe.As<ReadOnlyMemory<T>, Memory<T>>(ref readOnlyMemory); | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static ref T GetReference<T>(in Span<T> span) => ref span.Reference; |
There was a problem hiding this comment.
Have you tried whether it is worth it to have in here? (It was discussed in the issue.)
There was a problem hiding this comment.
Using in doesn't really help (at least for fast span). And if we don't inline the method, it actually hurts performance.
If I pass a Span using in, it seems to hurt performance in some cases:
[MethodImpl(MethodImplOptions.NoInlining)]
public static void CallTestMethodWithIn(Span<int> span)
{
int temp = TestMethodWithIn(span);
}
[MethodImpl(MethodImplOptions.NoInlining)]
public static void CallTestMethodWithoutIn(Span<int> span)
{
int temp = TestMethodWithoutIn(span);
}
[MethodImpl(MethodImplOptions.NoInlining)]
public static int TestMethodWithIn(in ReadOnlySpan<int> span)
{
int val = 0;
for(int i = 0; i < span.Length; i++)
val ^= span[i];
return val;
}
[MethodImpl(MethodImplOptions.NoInlining)]
public static int TestMethodWithoutIn(ReadOnlySpan<int> span)
{
int val = 0;
for (int i = 0; i < span.Length; i++)
val ^= span[i];
return val;
}Runtime (s):
CallTestMethodWithoutIn: 6.3821768
CallTestMethodWithIn: 6.8055258
There is an extra compare and jump in the loop body if I pass the Span with in.

| public static ref T GetReference<T>(in Span<T> span) => ref span.Reference; | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static ref readonly T GetReference<T>(in ReadOnlySpan<T> span) => ref span.Reference; |
There was a problem hiding this comment.
I do not think the Reference property will get inlined here for the class case. We may want to make the field internal instead.
There was a problem hiding this comment.
The ByReference<T> _pointer field?
There was a problem hiding this comment.
Yes, it looks like without AggressiveInlining it doesn't get inlined.
| object obj = readOnlyMemory.GetObjectStartLength(out int index, out int length); | ||
| if (index < 0) | ||
| { | ||
| if (((OwnedMemory<T>)obj).TryGetArray(out var segment)) |
There was a problem hiding this comment.
Can this be just return (((OwnedMemory<T>)obj).TryGetArray(out var arraySegment) ?
There was a problem hiding this comment.
I see - you need to add the local index.
* Add GetReference and TryGetArray to MemoryMarshal * Marking GetReference with AggressiveInlining * Do not use ByReference as a return type. * Addressing PR feedback. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add GetReference and TryGetArray to MemoryMarshal * Marking GetReference with AggressiveInlining * Do not use ByReference as a return type. * Addressing PR feedback. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add GetReference and TryGetArray to MemoryMarshal * Marking GetReference with AggressiveInlining * Do not use ByReference as a return type. * Addressing PR feedback.
* Add GetReference and TryGetArray to MemoryMarshal * Marking GetReference with AggressiveInlining * Do not use ByReference as a return type. * Addressing PR feedback. Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
* Add GetReference and TryGetArray to MemoryMarshal * Marking GetReference with AggressiveInlining * Do not use ByReference as a return type. * Addressing PR feedback. Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
* Add GetReference and TryGetArray to MemoryMarshal * Marking GetReference with AggressiveInlining * Do not use ByReference as a return type. * Addressing PR feedback. Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
* Add GetReference and TryGetArray to MemoryMarshal * Marking GetReference with AggressiveInlining * Do not use ByReference as a return type. * Addressing PR feedback. Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

Sets up the ground work for:
https://github.com/dotnet/corefx/issues/25412
https://github.com/dotnet/corefx/issues/25615
Test updates and changes in corefx
to follow after.dotnet/corefx#25789Following the staging plan from here: https://github.com/dotnet/corefx/issues/23881#issuecomment-343767740
Doing it this way will avoid the need for complex staging or things being on the floor for extensive periods of time.
cc @jkotas, @stephentoub, @KrzysztofCwalina