Implement IList<T> for EmptyPartition, RangeIterator and RepeatIterator.

[aalmada]

Empty

Enumerable.Empty<T> has two implementations. The SizeOpt version returns Array.Empty<T>, while the SpeedOpt version returns EmptyPartition<T>.Instance.

Besides the different optimizations, there's another difference: Array.Empty<T> returns an implementation of IList<T>, while EmptyPartition<T>.Instance returns an implementation of IEnumerable<T>.

This difference may cause some confusion as it behaves differently in different platforms, not only between different versions, as already reported in #32645.

@natemcmaster This PR adds the implementation of IList<T> as discussed on #32645. It also adds IReadOnlyList<T>.

Range and Repeat

These LINQ operations return implementations of Iterator<T> which implements IEnumerable<T>. But, for both these operations, Count is well known and the items can be inferred from the index value. This means that these iterators can implement IList<T>.

The advantage is that ICollection<T> and IList<T> allow much better performance than IEnumerable<T> in many situations. Many optimizations based on these interfaces can be found in this same repository and in many third-parties.

Some of these cases may be overloaded by even better optimizations (SpeedOpt and SizeOpt) but these are not used in all platforms. Still, these changes, take advantage of existing code. No new cases are created.

This PR adds implementation of IList<T> and IReadOnlyList<T> to the iterators RangeIterator and RepeatIterator<T>.

Both Range and Repeat return Empty<T>() when count is zero. That's one other reason why it's important EmptyIterator<T> also implements IList<T>, reducing confusion.

ToList()

One of the cases that is overloaded in SpeedOpt, is the ToList()method for both RangeIterator and RepeatIterator<T>. They simply create a List<T> and add the elements to it. Here's the implementation for Range:

public List<int> ToList()
{
   List<int> list = new List<int>(_end - _start);
   for (int cur = _start; cur != _end; cur++)
   {
       list.Add(cur);
   }

   return list;
}

Although the length is passed as the capacity for the new List<T>, the Add() method still performs version increment, buffer length validation and size increment. This is performed for every element, which is unnecessary and penalizing.

public void Add(T item)
{
    _version++;
    T[] array = _items;
    int size = _size;
    if ((uint)size < (uint)array.Length)
    {
        _size = size + 1;
        array[size] = item;
    }
    else
    {
        AddWithResize(item);
    }
}

As an alternative, the List<T>(IEnumerable<T>) constructor can be used, which has the following implementation:

public List(IEnumerable<T> collection)
{
    if (collection == null)
        ThrowHelper.ThrowArgumentNullException(ExceptionArgument.collection);

    if (collection is ICollection<T> c)
    {
        int count = c.Count;
        if (count == 0)
        {
            _items = s_emptyArray;
        }
        else
        {
            _items = new T[count];
            c.CopyTo(_items, 0);
            _size = count;
        }
    }
    else
    {
        _size = 0;
        _items = s_emptyArray;
        using (IEnumerator<T> en = collection.GetEnumerator())
        {
            while (en.MoveNext())
            {
                Add(en.Current);
            }
        }
    }
}

When collection is a ICollection<T>, which is now the case, it simply allocates an array, calls ICollection<T>.CopyTo() and sets the size.

This PR replaces the for loop with List.Add() calls by a single call to the List<T>(IEnumerable<T>) constructor.

Benchmarks

These are the benchmarks for Range with multiple count values (0, 1, 10, 100, 1.000, 10.000). The first line values are for master branch and the second for this PR branch.

BenchmarkDotNet=v0.11.3.1003-nightly, OS=macOS Mojave 10.14.4 (18E226) [Darwin 18.5.0]
Intel Core i5-7360U CPU 2.30GHz (Kaby Lake), 1 CPU, 4 logical and 2 physical cores
.NET Core SDK=3.0.100-preview4-011223
  [Host]     : .NET Core 3.0.0-preview4-27615-11 (CoreCLR 4.6.27615.73, CoreFX 4.700.19.21213), 64bit RyuJIT
  Job-CFRYCL : .NET Core e70c7a22-15eb-4e75-85bf-9200f5f40b1a (CoreCLR 4.6.27701.72, CoreFX 4.700.19.25201), 64bit RyuJIT
  Job-XREZYG : .NET Core 0ac558bb-ff95-4358-9e42-24ac416bfb32 (CoreCLR 4.6.27624.71, CoreFX 4.700.19.23001), 64bit RyuJIT

Runtime=Core  IterationTime=250.0000 ms  MaxIterationCount=20  
MinIterationCount=15  WarmupCount=1  

Method	Toolchain	Count	Mean	Error	StdDev	Median	Min	Max	Ratio	MannWhitney(10%)	RatioSD	Gen 0/1k Op	Gen 1/1k Op	Gen 2/1k Op	Allocated Memory/Op
ToList	/corefx/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	0	20.25 ns	1.2950 ns	1.4913 ns	20.13 ns	18.46 ns	22.86 ns	1.04	Same	0.08	0.1958	-	-	32 B
ToList	/corefx_upstream/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	0	19.47 ns	0.5175 ns	0.5959 ns	19.28 ns	18.77 ns	20.66 ns	1.00	Base	0.00	0.1958	-	-	32 B
ToList	/corefx/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	1	52.89 ns	0.6164 ns	0.5464 ns	52.76 ns	52.07 ns	53.81 ns	1.45	Slower	0.02	0.3917	-	-	64 B
ToList	/corefx_upstream/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	1	36.52 ns	0.2953 ns	0.2762 ns	36.45 ns	36.23 ns	37.18 ns	1.00	Base	0.00	0.3916	-	-	64 B
ToList	/corefx/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	10	63.85 ns	0.5476 ns	0.4855 ns	63.83 ns	63.10 ns	64.83 ns	1.07	Same	0.02	0.5873	-	-	96 B
ToList	/corefx_upstream/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	10	59.60 ns	0.9767 ns	0.8658 ns	59.39 ns	58.63 ns	61.37 ns	1.00	Base	0.00	0.5874	-	-	96 B
ToList	/corefx/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	100	200.93 ns	2.2097 ns	1.9589 ns	200.56 ns	198.80 ns	205.87 ns	0.69	Faster	0.04	2.7855	-	-	456 B
ToList	/corefx_upstream/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	100	289.58 ns	19.1209 ns	18.7793 ns	281.08 ns	277.56 ns	331.04 ns	1.00	Base	0.00	2.7853	-	-	456 B
ToList	/corefx/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	1000	1,450.43 ns	14.6100 ns	13.6662 ns	1,449.04 ns	1,423.05 ns	1,477.32 ns	0.60	Faster	0.01	24.3859	-	-	4056 B
ToList	/corefx_upstream/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	1000	2,426.86 ns	22.6250 ns	18.8929 ns	2,420.40 ns	2,397.13 ns	2,461.57 ns	1.00	Base	0.00	24.3898	-	-	4056 B
ToList	/corefx/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	10000	8,182.43 ns	97.2001 ns	90.9211 ns	8,176.24 ns	8,079.06 ns	8,427.27 ns	0.46	Faster	0.01	57.1382	7.1382	-	40056 B
ToList	/corefx_upstream/artifacts/bin/testhost/netcoreapp-OSX-Release-x64/shared/Microsoft.NETCore.App/9.9.9/corerun	10000	17,766.04 ns	98.3112 ns	91.9603 ns	17,780.30 ns	17,573.70 ns	17,901.24 ns	1.00	Base	0.00	57.1206	7.0781	-	40056 B

Please note that when count is zero, Range return Empty<int>(). The new version has the same performance.

For the other cases where RangeIterator is returned, there is an initialization overhead due to the cast and argument validation. It's noticeable for very small values of count. It's 45% times slower when count is 1.

This overhead dilutes quickly when count increases. It takes more or less the same time when count is 10. It's 30% faster when count is 100, 40% faster when count is 1.000 and 54% faster when count is 10.000.

In absolute values, this means that, it's only 16ns slower when count is 1 but almost 10.000ns faster when count is 10.000.

There's no memory overhead added.

[stephentoub]

as already reported in #32645

Why would implementing IList<T> be sufficient to avoid this "confusion"? Can you share some examples where developers are actually confused by this? If we truly believe that's a problem, why do we believe folks expect it to implement IList<T> but not actually be a T[]?

We make no guarantees about the concrete type that's returned from these APIs, nor what interfaces they may or may not implement. Anyone who's casting should be doing so with a type check and a fallback.

But if we were going to make a change to Empty<T> because of concerns about the concrete type returned and its interface implementations, then wouldn't we just go back to using Array.Empty<T>? I don't think it makes sense to start adding back such interfaces piecemeal, and even then not actually capture the full expectation and avoid that confusion.

The advantage is that ICollection and IList allow much better performance than IEnumerable in many situations. Many optimizations based on these interfaces can be found in this same repository and in many third-parties.

I just spent half an hour looking through a bunch of code for use of Enumerable.Range(...) and Enumerable.Repeat(...). I found very few cases where they were directly followed by ToList and where performance mattered. At least in the code I looked at, by far the most common thing to follow Range was a Select, so I opened #37410. After that was ToArray, which is already handled by the RangeIterator implementation. Enumerable.Repeat itself is very rare in anything but test code I looked at.

but these are not used in all platforms

They're not used in all platforms to avoid a bunch of additional generic instantiations and interface implementations when running on AOT platforms that care a lot about binary size. Adding IList<T> implementations has the same problem.

I do appreciate your interest in contributing here. LINQ is an enticing area to add optimizations, with practically an infinite number of them possible. But it's infeasible for the implementation to provide optimizations for every possible combination of operator and input source, in terms of implementation and subsequent code maintainence, in terms of binary size, and in terms of the tradeoffs that result, and so we have basic implementations that work with everything and then only special-case those patterns we've found to be common and that are expected to make a meaningful difference on consuming code, and in particular in consuming code where such improvements actually matter. I'm not convinced this meets that bar. If you have real examples where it would, please do share; I'm happy to be convinced otherwise.

[aalmada]

LINQ is an enticing area to add optimizations, with practically an infinite number of them possible. But it's infeasible for the implementation to provide optimizations for every possible combination of operator and input source, in terms of implementation and subsequent code maintainence, in terms of binary size, and in terms of the tradeoffs that result, and so we have basic implementations that work with everything and then only special-case those patterns we've found to be common and that are expected to make a meaningful difference on consuming code, and in particular in consuming code where such improvements actually matter. I'm not convinced this meets that bar. If you have real examples where it would, please do share; I'm happy to be convinced otherwise.

@stephentoub I'm well aware that LINQ is a beast. I've been trying to contribute for while but it's been hard to find a spot where it doesn't break everything. Where, it can radically improves big data without affecting tiny collections.

I'm also well aware that Range().ToArray() or Range().Select().ToArray() are way more common than Range().ToList(). This was a first attempt of a simple contribution, for the sake of code completeness and to evaluate acceptance criteria.

I would think that #32645 is an example of someone getting confused. Also, I hope my comment there clarifies my opinion.

I've been working on my own "green field" version of LINQ where I test my own ideas. I'm not worried about binary size but with performance on big collections.

Looks like I'm going to stick to my work only but you're welcome to comment there.

[stephentoub]

I've been trying to contribute for while but it's been hard to find a spot where it doesn't break everything. Where, it can radically improves big data without affecting tiny collections.

The contributions really are appreciated. As you suggest, though, it can be difficult to find places to make meaningful improvements. For several years we had a few contributors actively optimizing various parts of LINQ, and so lots of the low-hanging fruit has already been picked. Optimizations at this point generally involve tradeoffs that need to be scrutizined or alternatively necessitate data to demonstrate why the cost (e.g. extra code) is worth it in terms of the payoff.

I would think that #32645 is an example of someone getting confused. Also, I hope my comment there clarifies my opinion.

The question there was, "is it reasonable for (IList<T>)Empty<T>() to keep working?" The same question could be asked with "is it reasonable for (T[])Empty<T>() to keep working?" I think the answer needs to be the same, as if folks are expecting the type to be the same as it was previously, then I don't know why that would apply to a subset of what it implemented.

Looks like I'm going to stick to my work

Ok. Best of luck with your project. If you find things that you think would be valuable contributions back to System.Linq, we'd welcome the discussion.

Thanks!

[GSPP]

When Empty<T>() was made to return an array it was entirely predictable that people would (mistakenly) take a dependency on that. It is generally my view that the very first implementation in the framework should intentionally try to break such dependencies as much as possible. Empty<T>() should have taken care to not return anything deriving from IList.

Just as an example, changing String.GetHashCode() was hard because it was deterministic in 1.0. It would have been better to randomize it in 1.0 using a very simple scheme (e.g. return actualHash ^ staticHash;) to break people from taking a dependency.

Another example is Random where people (of course) now depend on the exact sequence of values. Therefore, we are stuck with a terrible algorithm that is slow, has a large state and produces bad numbers. It should have used the same trick that I just proposed for string.

If Empty<T>() actually returns an IList this becomes effectively contractual. Therefore, the method is pointless. People can directly use Array.Empty<T>(). Or, the return type could directly be made to promise IList.