MemoryExtensions.AsSpan/AsMemory throws different exception than {ReadOnly}Span.Slice

[gfoidl]

In

runtime/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs

Lines 135 to 154 in dbb05eb

	public static ReadOnlySpan<char> AsSpan(this string? text, int start, int length)
	{
	if (text == null)
	{
	if (start != 0 || length != 0)
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	return default;
	}
	#if TARGET_64BIT
	// See comment in Span<T>.Slice for how this works.
	if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)text.Length)
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	#else
	if ((uint)start > (uint)text.Length || (uint)length > (uint)(text.Length - start))
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	#endif
	return new ReadOnlySpan<char>(ref Unsafe.Add(ref text.GetRawStringData(), (nint)(uint)start /* force zero-extension */), length);
	}

ExceptionArgument.start is used, but length may be the faulty argument.

runtime/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs

Lines 350 to 362 in dbb05eb

	public ReadOnlySpan<T> Slice(int start, int length)
	{
	#if TARGET_64BIT
	// See comment in Span<T>.Slice for how this works.
	if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
	ThrowHelper.ThrowArgumentOutOfRangeException();
	#else
	if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
	ThrowHelper.ThrowArgumentOutOfRangeException();
	#endif
	return new ReadOnlySpan<T>(ref Unsafe.Add(ref _pointer.Value, (nint)(uint)start /* force zero-extension */), length);
	}

only throws ArgumentOutOfRangeException without specifying the paramname.
{RO}S ctor does it the same way.

Similar for AsMemory(this string? text, int start, int length);.

Should the behavior of MemoryExtensions.AsSpan/AsMemory be aligned to the one of Slice?

For reference see also #53463 (comment)

Tagging subscribers to this area: @GrabYourPitchforks, @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In

runtime/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs

Lines 135 to 154 in dbb05eb

	public static ReadOnlySpan<char> AsSpan(this string? text, int start, int length)
	{
	if (text == null)
	{
	if (start != 0 || length != 0)
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	return default;
	}
	#if TARGET_64BIT
	// See comment in Span<T>.Slice for how this works.
	if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)text.Length)
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	#else
	if ((uint)start > (uint)text.Length || (uint)length > (uint)(text.Length - start))
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	#endif
	return new ReadOnlySpan<char>(ref Unsafe.Add(ref text.GetRawStringData(), (nint)(uint)start /* force zero-extension */), length);
	}

ExceptionArgument.start is used, but length may be the faulty argument.

runtime/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs

Lines 350 to 362 in dbb05eb

	public ReadOnlySpan<T> Slice(int start, int length)
	{
	#if TARGET_64BIT
	// See comment in Span<T>.Slice for how this works.
	if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
	ThrowHelper.ThrowArgumentOutOfRangeException();
	#else
	if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
	ThrowHelper.ThrowArgumentOutOfRangeException();
	#endif
	return new ReadOnlySpan<T>(ref Unsafe.Add(ref _pointer.Value, (nint)(uint)start /* force zero-extension */), length);
	}

only throws ArgumentOutOfRangeException without specifying the paramname.
{RO}S ctor does it the same way.

Similar for AsMemory(this string? text, int start, int length);.

Should the behavior of MemoryExtensions.AsSpan/AsMemory be aligned to the one of Slice?

For reference see also #53463 (comment)

Author:	gfoidl
Assignees:	-
Labels:	area-System.Memory, untriaged
Milestone:	-

[GrabYourPitchforks]

Initial thoughts: If we wanted to unify this, I'd prefer to remove the argument entirely rather than try to figure out which of "length" or "offset" is correct. Since this is such a hot code path we don't have the luxury of inserting the extra few instructions it'd take to figure out a better parameter name.

[xtqqczze]

Removing the argument entirely could be a good option, but I thought that instead we could call a new throw helper method:
ThrowHelper.ThrowAsSpanArgumentOutOfRangeExceptions(text, start, length)

AsSpan1 and AsSpan2 below have the same length of bytecode, but in theory performance should be improved in the latter, as the the reg-reg movs are effectively free due to register renaming:

https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcAMACOSB0AIgJZgDmAdgPYTDEDGE+AwlQCYCmAghWADYBPCMQgBudFlwEASgFcKdKBxZUoAB2J8OAJwDKOgG4MOYiTjz45C4kvwBJBTqpr92o/RPi06AMQVZfHxgAEZa2By8oRzoEgDMUgBsuAjYALIcUFTaAgCiAB7AEcJUFBDYAN7o2NW48XhJ0hxgbADyFIK6amAUADz0ABZg2gB82FwQnd0AFMD9IlKYAPzYhQUANNjECti0Q8AbW8DYWhRkswCUVTWVaDV3mwBm2DMcBdgAvO/Y/oGXt/fVG4AgHEJ5TXbaI4AQi+OBgMGOETO/WwMOwmD+wKx2AAKv1tFQAO4ACQ4fDUOnweIJhK42jIsiUChasmALQe0m6ZA4+Q8ajoJSmvI4/OIJTpDKZwHwEOA5y82JqcAA7NhOA8wAFgArgQBfGL/EFgqayQ7nWXYUYms2raUAGSRs2w8OepoU5xOyMtrrNLwK+AdpydAFodsA9udMYrcfiiaTyZTqUSJYyImzWezOaceXk+QKKELcyL8ympTLw5D5QasSq0hksgJUkMIIM+CxtE1CpMCx2ngBVUpgB7KLhsNhTXt1zLZJvaFv8fAAcQ4wEaw47FA8fUGIz9co2UwovrdcotAHoAFTYB5ZDzYABeTmDr0KpTFFGwF7P5w2nouOuqfVDVqRJsEaZo2g6LpegGIZRnGbsEBmOYyjwJYVhfA5tllLCjj/foo0BK4jWeW0Pi+H4+EI+4gWjUFngtNE4QRfDUVhajoxjGl4wpbQqVjWkJmg0s0xZNkOS5HM83fCA9w2HDESDAiAOxWt1U1PhtWI+4gO0u56Kma13QtK0T3OW0A0dFEXSMuVWNM30LMDL1Q1lSM9OBJMSTJXj+JpBDhPpVNmQzCTs2FUUSlk215IrfZFORKtvGAu5a3SadG2bVt207DhuwnDh+0HYd8FHcdJ3ShtZ3nNtl1XQqdAiLdYN3W0f2eI93Vs804uwS9r1vDgHyfF8infT9v1/KykruIDdLQQ4dF4Phwn8KBsAiksgqldBaOqAB9TA1m0nDtPw460HmuA6iQJJEC4uMfJ0CptIAbUIKgTAAOSoergFkbQKAAXW0xbAf4UC4AAFge2lttE0Ks25Tb30LaTxXh7YhklNNqL2+5Zhpb4OEJbAfuAOx1C0KUODYFHBRmmogLud7PogcnGn+wGQeAsHlshmGvIC7oRJC8Skak4sZKmNDlhizZsLi3CEoubT8buQmiWJ0nycptRqbTWn6YLRnAPQXUgA=

[GrabYourPitchforks]

@xtqqczze Normally I think that would be an ok idea, but these methods are inlined into their callers, which means that registers don't always match up as one would expect. And, if literal integer values are used, registers might be skipped entirely. Now this new code introduces additional shuffling beyond what previously existed.

To see this in action, define the two methods below, then mark AsSpan and AsSpan2 as aggressive-inlining.

    private static ReadOnlySpan<char> DoIt(string foo)
    {
        return AsSpan(foo, 3, 4);
    }

    private static ReadOnlySpan<char> DoIt2(string foo)
    {
        return AsSpan2(foo, 3, 4);
    }

In this scenario, DoIt is 62 bytes of codegen (x64), while DoIt2 is 71 bytes of codegen (x64).

[gfoidl]

Let's just remove the paramName, and go for performance. If the caller needs to distinguish between start, and length the caller can provide the necessary code*.

* in practice I've never encountered such a need for span, and friends. These cases (in user-code) should be covered by tests anyway, so any AOR-exception can be examined and fixed.

[xtqqczze]

Also:

runtime/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs

Lines 109 to 122 in c0ed319

	public static ReadOnlySpan<char> AsSpan(this string? text, int start)
	{
	if (text == null)
	{
	if (start != 0)
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	return default;
	}
	if ((uint)start > (uint)text.Length)
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	return new ReadOnlySpan<char>(ref Unsafe.Add(ref text.GetRawStringData(), (nint)(uint)start /* force zero-extension */), text.Length - start);
	}

runtime/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs

Lines 333 to 339 in c0ed319

	public ReadOnlySpan<T> Slice(int start)
	{
	if ((uint)start > (uint)_length)
	ThrowHelper.ThrowArgumentOutOfRangeException();
	return new ReadOnlySpan<T>(ref Unsafe.Add(ref _pointer.Value, (nint)(uint)start /* force zero-extension */), _length - start);
	}

In this case ExceptionArgument.start in AsSpan is meaningful, although inconsistent with Span<T>.Slice.

If we keep the ExceptionArgument we could create new ThrowHelper.ThrowStartArgumentOutOfRangeException for smaller code size.

[xtqqczze]

If we keep the ExceptionArgument we could create new ThrowHelper.ThrowStartArgumentOutOfRangeException for smaller code size.

See xtqqczze@fe0ea4f for an example (also on this branch there are a few ThrowHelper changes I think would be good for performance).