Reduce asm size of CollectionsMarshal.AsSpan

[stephentoub]

Using the span constructor brings with it some branches and code we don't need due to knowledge of List<T>'s implementation, in particular that a) it always has an array (even if empty), and b) its array is invariant.

Erroneous use of AsSpan while the list is being mutated could have also led to an ArgumentOutOfRangeException from the span's constructor, which doesn't make much sense in this context. I've changed it to be an InvalidOperationException about the invalid concurrent use.

Method	Toolchain	Code Size
AsSpan	\main\corerun.exe	99 B
AsSpan	\pr\corerun.exe	58 B
Sum	\main\corerun.exe	112 B
Sum	\pr\corerun.exe	75 B

usingBenchmarkDotNet.Attributes;usingBenchmarkDotNet.Running;usingSystem.Runtime.InteropServices;BenchmarkSwitcher.FromAssembly(typeof(Tests).Assembly).Run(args);[DisassemblyDiagnoser]publicclassTests{privatereadonlyList<string>_list=new(){"a","b","c"};[Benchmark]publicSpan<string>AsSpan()=>CollectionsMarshal.AsSpan(_list);[Benchmark]publicintSum(){inttotal=0;foreach(stringsinCollectionsMarshal.AsSpan(_list)){total+=s.Length;}returntotal;}}

[ghost]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Using the span constructor brings with it some branches and code we don't need due to knowledge of List<T>'s implementation, in particular that a) it always has an array (even if empty), and b) its array is invariant.

Erroneous use of AsSpan while the list is being mutated could have also led to an ArgumentOutOfRangeException from the span's constructor, which doesn't make much sense in this context. I've changed it to be an InvalidOperationException about the invalid concurrent use.

Method	Toolchain	Code Size
AsSpan	\main\corerun.exe	99 B
AsSpan	\pr\corerun.exe	58 B
Sum	\main\corerun.exe	112 B
Sum	\pr\corerun.exe	75 B

usingBenchmarkDotNet.Attributes;usingBenchmarkDotNet.Running;usingSystem.Runtime.InteropServices;BenchmarkSwitcher.FromAssembly(typeof(Tests).Assembly).Run(args);[DisassemblyDiagnoser]publicclassTests{privatereadonlyList<string>_list=new(){"a","b","c"};[Benchmark]publicSpan<string>AsSpan()=>CollectionsMarshal.AsSpan(_list);[Benchmark]publicintSum(){inttotal=0;foreach(stringsinCollectionsMarshal.AsSpan(_list)){total+=s.Length;}returntotal;}}

Author:	stephentoub
Assignees:	stephentoub
Labels:	area-System.Runtime.InteropServices
Milestone:	-

[jkotas]

Why is the JIT not able to optimize the original code well?

cc @dotnet/jit-contrib

[stephentoub]

Why is the JIT not able to optimize the original code well?

For starters I assume because it doesn't know that it can elide the null check on the array and elide the variance check on the array, since it's not privvy to the information about how the list was constructed.

[EgorBo]

For the variance check, minimal repro:

publicclassMyList<T>{publicT[]arr;}boolTest(MyList<string>list)=>list.arr.GetType()==typeof(string[]);

codegen:

; Method Program:Foo(MyList`1[System.String]):ubyte:this (FullOpts)G_M55733_IG01:G_M55733_IG02:movrax, gword ptr [rdx+0x08]movrcx,0xD1FFAB1E ; System.String[]cmp qword ptr [rax],rcx sete almovzxrax,alG_M55733_IG03:ret; Total bytes of code: 24

I bet it lost generic context somewhere during opts..

Querying runtime about current class of field MyList`1[System.__Canon]:arr (declared as System.__Canon[])
Field's current class not available
[000010] ---XG------ * RETURN int [000009] ---XG------ \--* EQ int [000008] #--XG------ +--* IND long [000002] n--XG------ | \--* IND ref [000001] ---X------- | \--* FIELD_ADDR byref MyList`1[System.__Canon]:arr
[000000] ----------- | \--* LCL_VAR ref V00 arg0 [000004] H---------- \--* CNS_INT(h) long 0xd1ffab1e class

[stephentoub]

For the variance check

Even if that's fixed for string, though, I imagine the JIT would have a lot more trouble doing it for an arbitrary non-sealed reference type.

[EgorBo]

For the variance check

Even if that's fixed for string, though, I imagine the JIT would have a lot more trouble doing it for an arbitrary non-sealed reference type.

Sure, but looks like it's should be a general goodness to improve this in JIT too if possible

[stephentoub]

Sure, but looks like it's should be a general goodness to improve this in JIT too if possible

Agreed. Just commenting that this change still improves upon what I imagine the JIT will be able to safely do, unless we build knowledge of List<T> into it directly.

[jkotas]

this change still improves upon what I imagine the JIT will be able to safely

Yea, I agree. LGTM.

[EgorBo]

For the variance check

Even if that's fixed for string, though, I imagine the JIT would have a lot more trouble doing it for an arbitrary non-sealed reference type.

Sure, but looks like it's should be a general goodness to improve this in JIT too if possible

Handled via #96794