IDE0305 recommends collection expression that results in worse IL #70656
Comments
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
Compiler is allowed to, and should use the best emit here. For bcl types, that should be ToArray. Tagging @RikkiGibson |
|
I agree that we should fix by improving emit here. It sounds like this would be a special case for a collection-expr of a single spread expression? Are there any other possible spread types where we should be looking for a ToArray method? e.g. Also, suppose we have a similar case |
|
We should special case as many reasonable cases as possible. But we can do it as bug fixes imo. Generally, if it's a trivial check (checking a few types and a few elements), then we should do it. Ideally in a way that is easy to maintain and enhance in the future! :-) |
RikkiGibson
added
Area-Compilers
Code Gen Quality
|
I was hoping to take this optimization and generalize it somewhat: instead of just improving the situation for collection-exprs like I thought we could do this with 2 changes:
I tried to benchmark to prove out the viability of this. However, I was surprised by the results, especially that the ToArray case doesn't seem to be allocating. I think there might be something wrong with my benchmark. Results and source below: edit: out of date.
using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
namespace MyBenchmarks
{
[MemoryDiagnoser]
public class CopyToBenchmarks
{
[Params(0, 10, 10000)]
public int N;
private readonly int[] data;
public CopyToBenchmarks()
{
data = new int[N];
var random = new Random();
for (var i = 0; i < data.Length; i++)
{
data[i] = random.Next();
}
}
[Benchmark]
public int[] CopyTo()
{
int[] dest;
if (N == 0)
{
dest = Array.Empty<int>();
}
else
{
dest = new int[N];
data.CopyTo(dest.AsSpan());
}
return dest;
}
[Benchmark]
public int[] ToArray()
{
int[] dest = data.AsSpan().ToArray();
return dest;
}
}
public class Program
{
public static void Main(string[] args)
{
var summary = BenchmarkRunner.Run<CopyToBenchmarks>();
}
}
}Any insight on what could be happening here would be appreciated. I also tried comparing JIT ASM for ToArray versus CopyTo and...yeah, I found they looked totally different and didn't know how to meaningfully compare them. |
|
@RikkiGibson are you sure the N is set before the constructor runs? If not, you will always have empty arrays. |
|
That was it :) Including new results and benchmark below.
using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
namespace MyBenchmarks
{
[MemoryDiagnoser]
public class CopyToBenchmarks
{
[Params(0, 10, 10000)]
public int N;
private int[] data = null!;
[GlobalSetup]
public void Setup()
{
data = new int[N];
var random = new Random();
for (var i = 0; i < data.Length; i++)
{
data[i] = random.Next();
}
}
[Benchmark]
public int[] CopyTo()
{
int[] dest;
if (N == 0)
{
dest = Array.Empty<int>();
}
else
{
dest = new int[N];
data.CopyTo(dest.AsSpan());
}
return dest;
}
[Benchmark]
public int[] LinearAssignTo()
{
int[] dest;
if (N == 0)
{
dest = Array.Empty<int>();
}
else
{
dest = new int[N];
for (var i = 0; i < N; i++)
dest[i] = data[i];
}
return dest;
}
[Benchmark]
public int[] ToArray()
{
int[] dest = data.AsSpan().ToArray();
return dest;
}
}
public class Program
{
public static void Main(string[] args)
{
var summary = BenchmarkRunner.Run<CopyToBenchmarks>();
}
}
}The perf difference seems pretty close between CopyTo and ToArray. If it seems reasonable to everybody I might just focus my implementation efforts on using CopyTo for arrays, spans and lists (using CollectionsMarshal to copy to the span referencing the list's backing array.) |
That is reasonable to me. |
From a throughput perspective, yup, the difference is minimal. From a code size perspective, it'd be nice if ToArray could be used for |
This comment was marked as off-topic.
This comment was marked as off-topic.
They do. The number for |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Just noticed this (sharplab): public static IList<int> M() => [1, 2, 8, 4];List<int> list = new List<int>();
CollectionsMarshal.SetCount(list, 4);
Span<int> span = CollectionsMarshal.AsSpan(list);
int num = 0;
span[num] = 1;
num++;
span[num] = 2;
num++;
span[num] = 8;
num++;
span[num] = 4;
num++;
return list;It seems there's a fair amount of extra IL just to keep track of that index that's incremented every time, which also makes it not a constant (which might be less useful for the JIT). Is there any specific reason why Roslyn is doing this rather than just emitting: List<int> list = new List<int>();
CollectionsMarshal.SetCount(list, 4);
Span<int> span = CollectionsMarshal.AsSpan(list);
span[0] = 1;
span[1] = 2;
span[2] = 8;
span[3] = 4;
return list;The latter might even allow the JIT to vectorize some of these assignments, in theory (also the IL seems smaller too?). |
Yes. It was the simplest way to do code Gen, and we have not focused on optimizations. |
|
Thanks @Sergio0694, yes we should use constant indices for this case. I've created #71183 from your comment. |
|
For big, constant data |
|
Also wanted to show a comparison of AddRange, CopyTo, and ToList for Benchmark source[MemoryDiagnoser]
public class CopyToListBenchmarks
{
[Params(0, 10, 10000)]
public int N;
private int[] data = null!;
[GlobalSetup]
public void Setup()
{
data = new int[N];
var random = new Random();
for (var i = 0; i < data.Length; i++)
{
data[i] = random.Next();
}
}
[Benchmark]
public List<int> ToList()
{
List<int> dest = data.ToList();
return dest;
}
[Benchmark]
public List<int> AddRange()
{
List<int> dest = new(capacity: N);
dest.AddRange(data);
return dest;
}
[Benchmark]
public List<int> CopyTo()
{
List<int> dest = new();
CollectionsMarshal.SetCount(dest, N);
data.CopyTo(CollectionsMarshal.AsSpan(dest));
return dest;
}
[Benchmark]
public List<int> LinearAdd()
{
List<int> dest = new(capacity: N);
for (var i = 0; i < N; i++)
dest.Add(data[i]);
return dest;
}
[Benchmark]
public List<int> LinearSpanAssign()
{
List<int> dest = new();
CollectionsMarshal.SetCount(dest, N);
var span = CollectionsMarshal.AsSpan(dest);
for (var i = 0; i < N; i++)
span[i] = data[i];
return dest;
}
}
|
Version Used:
Version 17.9.0 Preview 1.0 [34231.268.main]
Steps to Reproduce:
IDE0305 recommends rewriting this to:
which would be fine if that still compiled down to a
list.ToArray()call, but instead it compiles down to the equivalent of:which is significantly less efficient, especially for longer lists where the entire copy would instead be done as a vectorized memcpy invocation.
It's also worse if the list was actually empty:
List<T>.ToArray()will return an empty array singleton if the count is 0, but the above code will end up always allocating a new empty array 😦cc: @cston, @CyrusNajmabadi
The text was updated successfully, but these errors were encountered: