runtime: Memory and ReadOnlyMemory validation errors not matching
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
- “is buffer null” == null argument exception
- “is offset negative or past the end of the buffer?” == argument out of range exception with offset as field referenced
- “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<T> 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|
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Comments: 52 (52 by maintainers)
you are correct Sir they should match the Span names, as it doesn’t matter what they are called for my purpose now, and … consistency
I fixed it ( I left the example code as Array, offset to make the point about what they are likely to be 😉 )
Do the argument names in DangerousCreate have to be different than what we use in the Span constructor or can we continue to use index/length?
Does that look better?
System.Runtime.InteropServices:shipit:
How about just adding a Dangerous/Unsafe factory for
{ReadOnly}Memory<T>that doesn’t do any argument validation at all, leaving it up to the call site? Then I can do my own argument validation if needed, and places where I know the arguments are valid I can skip the checks entirely. There are a bunch of places I’m seeing where, as I’m switching over from using array-based to memory-based calls, I’m having to construct{ReadOnly}Memory<T>instances fromT[]/offset/count triplets that I know are all valid.DangerousCreate is there for Span anyway, though its checking length (but not other params) https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/Span.cs#L108-L113
Yeah it looks from a quick tour around that it is the same for the non async methods that take span.
@ahsonkhan If you want I can update the top comment to be in proper API review form?