Memory and ReadOnlyMemory validation errors not matching

[Drawaes]

Updated as per the suggestion of a create method Updated with design from @stephentoub which allows all cases to be covered Updated with design change from @benaadams to allow type inference for T Put in namespace and removed the Create to leave only the Dangerous Create Added question around moving current Dangerous Create Method

Rationale

A major use case of [ReadOnly]Span/Memory is to replace handing around array buffers and their offsets and count.

One of the major benifits of the design as I see it as it moves bounds checks out to where the buffer is created which is excellent. However when upgrading legacy code there seems to be a blocker in that stream defines the triplet to be

public int SomeMethod(byte[] buffer, int offset, int count);

This is normally then teamed up with checks on

1. "is buffer null" == null argument exception
2. "is offset negative or past the end of the buffer?" == argument out of range exception with offset as field referenced
3. "is count > than buffer.length, or < 0 or count +offset > buffer.length" == argument out of range exception

The issue with the way it currently is, that for anything that takes that triplet you have to manually do validation on the inputs before creating a Memory or risk having exceptions with names that don't match.

This causes double validation to take place, once in the legacy code and once in the Memory creation. As Memory is often used on the "hot paths" of code it is a penalty for using memory.

Proposal

Add "unsafe" methods to create memory and readonly memory that avoid the extra checks. Then the checks can be maintained in the code with the same error messages and the double check penalty doesn't have to be paid.

namespace System.Runtime.InteropServices
{
    public static class Span
    {
        public static Span<T> DangerousCreate(T[] array, int start, int length);
    ...
    }
    // ... same for ReadOnlySpan<T>

    public static class Memory
    {
        public static Memory<T> DangerousCreate(T[] array, int start, int length);
    }

    // ... same for ReadOnlyMemory<T>
}

Usage

byte[] buffer;

var span = Span.DangerousCreate(buffer, offset, count);
// vs
var span = Span<byte>.DangerousCreate(buffer, offset, count);

Outstanding Questions

Should the existing method

[MethodImpl(MethodImplOptions.AggressiveInlining)]
[EditorBrowsable(EditorBrowsableState.Never)]
public static Span<T> DangerousCreate(object obj, ref T objectData, int length)

Be moved to the new types and should the IDE hiding be removed?

References

dotnet/corefx#24295 The Code this came up in (It means an extra validation path without it)

Below no longer matters as the legacy code can maintain it's own named validation.

|Class|ArrayName|Start|Length|
|---|---|---|---|
|FileStream|buffer|offset|count|
|NetworkStream|buffer|offset|count|
|BufferedStream|buffer|offset|count|
|MemoryStream|buffer|offset|count|
|StreamReader|buffer|index|count|
|StreamWriter|buffer|index|count|
|Stream|buffer|offset|count|
|SslStream|buffer|offset|count|

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/bHwCPVNQLwo?t=4584 (23 min duration)

[tarekgh]

I am closing this review per the latest discussion on this issue. here are some comments:

• We already have Span.DangerousCreate and we have another overlapping issue dotnet/corefx#26139 which proposing moving the Span.DangerousCreate to MemoryMarshal.DangerousCreate
• @Drawaes can still provide his E2E scenario measurements and we can consider it if it is worth doing something more (e.g. adding MemoryMarshal.DangerousCreate which support creating Memory objects too). Or suggest any change in the dotnet/corefx#26139. @ahsonkhan is already aware of that and he can update dotnet/corefx#26139 as needed. @Drawaes, feel free to post your measurement numbers here or in dotnet/corefx#26139