List-like union as struct to save allocation + virtual dispatch #472
Conversation
MoreLinq/ListLike.cs
Outdated
| T this[int index] { get; } | ||
| } | ||
| readonly IList<T> _l; | ||
| readonly IReadOnlyList<T> _r; |
There was a problem hiding this comment.
These member names are quite cryptic.
There was a problem hiding this comment.
Tried to address this with 064fb61. Perhaps rw and rx are still cryptic, but wanted to keep them the same length (trying to hard?) so these are *nix-inspired (rw = read-write, rx = read-only).
| @@ -21,34 +21,35 @@ namespace MoreLinq | |||
| using System.Collections.Generic; | |||
|
|
|||
| /// <summary> | |||
| /// Represents an list-like (indexable) data structure. | |||
| /// Represents a union over list types implementing either | |||
There was a problem hiding this comment.
Instead of a union, what about a delegating tuple?
static class ListLike
{
public static (Func<int> GetCount, Func<int, T> GetItem) => (() => list.Count, (i) => list[i]);
public static (Func<int> GetCount, Func<int, T> GetItem) => (() => list.Count, (i) => list[i]);
}There was a problem hiding this comment.
This isn't syntactically correct, but I think I get your idea nonetheless. This would cause two closures and indirect calls so I have my doubts it's as optimal.
Conflicts resolved: - MoreLinq/ListLike.cs
Codecov Report
@@ Coverage Diff @@
## master #472 +/- ##
==========================================
- Coverage 92.39% 92.36% -0.04%
==========================================
Files 112 112
Lines 3434 3443 +9
Branches 1021 1023 +2
==========================================
+ Hits 3173 3180 +7
Misses 199 199
- Partials 62 64 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Benchmark code: [ParamsSource(nameof(Sources))]
public IEnumerable<int> Source { get; set; }
public IEnumerable<IEnumerable<int>> Sources
{
get
{
var xs = Enumerable.Range(1, 1_000_000);
yield return xs;
yield return xs.ToArray();
}
}
[Benchmark]
public void CountDown()
{
Source.CountDown(1, (_, _) => 0).Consume();
}Benchmarks for
Notice that the run with an array as a source is slower. Following is the benchmark for changes in this PR up to 064fb61:
The run with an array as a source is slightly faster. |
|
@atifaziz If you would, please compare your latest results to SuperLinq 4.6.0? I did a comparison against latest public release (3.3.2), and I show that SuperLinq is 40% faster on array and essentially equivalent (4% faster) on enumerables.
For Data: https://gist.github.com/viceroypenguin/ec71e7adb8124c455e8a2925bee1b987 |
|
I had already removed
Data: https://gist.github.com/viceroypenguin/ec71e7adb8124c455e8a2925bee1b987 |
I shared those benchmarks to get back to the comment from @fsateler, where he rightly challenged that the performance & allocation benefits were not demonstrated (trade-off being code legibility). So this PR is not about diff --git a/MoreLinq/CountDown.cs b/MoreLinq/CountDown.cs
index f7b8ade1..1f5f72da 100644
--- a/MoreLinq/CountDown.cs
+++ b/MoreLinq/CountDown.cs
@@ -63,17 +63,13 @@ static partial class MoreEnumerable
? IterateCollection(collectionCount)
: IterateSequence();
- IEnumerable<TResult> IterateList(ListLike<T> list)
+ IEnumerable<TResult> IterateList(IListLike<T> list)
{
- var countdown = Math.Min(count, list.Count);
+ var listCount = list.Count;
+ var countdown = Math.Min(count, listCount);
- for (var i = 0; i < list.Count; i++)
- {
- var cd = list.Count - i <= count
- ? --countdown
- : (int?) null;
- yield return resultSelector(list[i], cd);
- }
+ for (var i = 0; i < listCount; i++)
+ yield return resultSelector(list[i], listCount - i <= count ? --countdown : null);
}
IEnumerable<TResult> IterateCollection(int i)then it makes a stark improvement:
In fact, it's almost 15% faster than SuperLinq (which I'm guessing is just iterating the list via the enumerator).
So perhaps not? I haven't investigated in detail as to why, but if I were to take a somewhat educated guess, then I'd say iterating a sequence makes two virtual dispatches per loop iteration ( |
atifaziz
changed the title
There was a problem hiding this comment.
@fsateler I'm going to assume you don't have the time to review, so I will go ahead and merge this since there's been at least another pair of eyes (@viceroypenguin) over it. If there's any strong objection (at some future point) to some aspect or the whole refactoring then feel free to open a follow-up issue. Thanks!
This PR converts the list-like union via a class hierarchy to a
structfor the purpose of saving GC allocations and additional virtual hops. I don't expect there will ever be more than one allocation saved per list and per invocation of an operator so the savings there are really pathetic. The implementation also feels a bit clumsy but there is not much choice there.