Enumerable.Index/Select((T, int)) iterator classes #102314
Conversation
…2252) * Allows both methods to have their lengths obtained from Enumerable.TryGetNonEnumeratedCount(). * Improves performance of Enumerable.Index with an Array/IList/List source.
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-linq |
|
Oh dear, the tables turned out far less readable than it looked in preview, I don't know if I can do anything to make them look better. |
What binary size is being measured here, the System.Linq.dll assembly? While that's interesting, the more impactful size is assembly, e.g. if you publish a NativeAOT app that's using LINQ, say a Select operator somewhere, what does the addition of these types do to that NativeAOT published binary size? |
I originally thought file-size concerns were regarding |
|
In light of the inconclusive results, I think it probably isn't worth applying iterator optimizations to foreach ((int i, T item) in source.Index) { }And I wouldn't consider it common enough for people to chain it with |
|
I've been trying all day but had trouble compiling NativeAOT with the self-built However, my idea is that if performance and binary size stays more-or-less the same, isn't that already good enough? Perhaps the average use-case isn't optimized, but if |
It also replaces a simple implementation with a more complex one. Any change comes with added cost, including risk of potential regression so this would need to be outweighed with measurable benefits. |
|
That being said, I really appreciate your effort and the attention to detail that you put into this PR! I think we collectively learned something by it, so let this not dissuade you from similar experimentation in the future. |
If I may ask, what kind of potential regressions could you be referring to? All tests pass just fine, and performance is the same, or slightly better for |
No specific concerns, I'm just pointing out that every PR that we merge introduces risk and maintenance cost. If it doesn't move the needle substantially, we err towards not making the change at all. |
|
Closing per #102314 (comment). But thanks! |
This PR addresses #102252 by replacing the iterator methods for
Enumerable.IndexandEnumerable.Select(Func<TSource, int, TResult>overload) with iterator classes, which allows the number of elements to be extracted usingEnumerable.TryGetNonEnumeratedCount.There are two considerations to this change to focus on, which is how it affects binary size and enumeration performance compared to before.
Binary Size
To make up for the added iterator class,
Sizecompromises by reusing the iterator class forIndex, which has a slight overhead in delegate invocation. This happens to perfectly balance out, keeping the file size the same.Performance
The following benchmarks were ran on the following specs:
For clarity, here are the two benchmarks I had to add to
System.Linqwhich can also be found in this PR:Before:
After (Size):
After (Speed):
Before vs After (Size):
Before vs After (Speed):
I am admittedly very confused on why
SelectTwoArgsperforms much worse inSpeedwhen both are compiled identically.Personal Opinion
Enumerable.Indexvery clearly benefits from this, and I think it's a pretty open-and-shut case to include this change. The same cannot be said forSelect, or perhaps we need more hardware to benchmark this. If the new implementation proves insufficient, we can scrap theSelectimplementation but keep theIndexiterator class.Hopefully I did everything correctly, and by all means I welcome any suggestions or changes to make this better or faster.