Reduce linked node size in Enumerable.Append/Prepend/Union

[jamesqo]

• For Append / Prepend the count of previous iterators isn't needed by this iterator, so we can move that stuff from the nodes to the iterators which can be GC'd on successive linked Appends/Prepends.

• Similarly, all of the non-readonly state for Union of previous iterators isn't relevant for this iterator, so use SingleLinkedNode to only keep alive what we need (the sources). Additionally, all of the iterators have the same comparer so we only need to store the comparer in the latest node.

• Introduce Set.UnionWith for brevity and move SingleLinkedNode into its own file.

edit: Was finally able to run perf tests for this. GCs have increased, as expected, because there is a substantial reduction in leaked memory. Concat and Union see 50-60% reductions in leaked memory, Append and Prepend about 20%.

edit 2: Ignore the results for Concat, I hit a few complications there and the changes became pretty big. I'm going to save them for another PR.

/cc @JonHanna @VSadov

[jamesqo]

OK, I was finally able to post perf tests! 🎉 (see the description)

@JonHanna, @stephentoub, @VSadov PTAL

@karelz This can be un-marked as no merge.

[JonHanna]

I'm honestly not sure whether to consider this an improvement or not.

I'd be inclined to favour what benefits Concat and Union over what favours Append and Prepend and this seems to do the opposite in terms of speed if I'm reading the performance tests correctly. But maybe I shouldn't think that way (how much can we perhaps expect heavy use of Append and Prepend in the future?).

They all benefit in terms of memory, but if the objects produced were short-lived (which is v. common with linq) it would all be collected anyway, no? I'm not sure how to judge the value of something that saves in that regard but increases GC calls.

I does seem to have simplified things a bit, and that's always a good thing.

[jamesqo]

I'd be inclined to favour what benefits Concat and Union over what favours Append and Prepend and this seems to do the opposite in terms of speed if I'm reading the performance tests correctly.

The performance tests were only written to illustrate the impact on memory retention, not CPU time. It does not actually iterate any of the enumerables returned from the Linq methods. Iteration would quickly overshadow any impact on the time it takes to create the iterators.

I have encountered significant speed/memory regressions when calling Count/ToArray/ToList on Concat iterators, though, so you are correct in that sense. This is mostly because there are extra virtual calls and unnecessary re-walks of the linked list via GetEnumerable. I'm deciding to save those changes for a later PR because it's a lot more code and I'm no longer sure deleting the iterators is a clear win.

They all benefit in terms of memory, but if the objects produced were short-lived (which is v. common with linq) it would all be collected anyway, no? I'm not sure how to judge the value of something that saves in that regard but increases GC calls.

I actually thought that allowing older iterators to be GC'd was the reason you used SingleLinkedNode in your original Append/Prepend implementation, instead of putting the items directly in the iterator. Was that the case, or did you do it for some other reason?

BTW, there is also a benefit (albeit small) in terms of CPU time here too: since nodes have uniform type we no longer have to typecast when walking the linked list, since the tail is just represented as a null. https://github.com/dotnet/corefx/pull/15389/files#diff-49756bcd3a3a836bf3963f982c6af0f8L253

[JonHanna]

Ah. I see where you're going now.

So all that's left from what I've said above is that it seems to have simplified things, which is good 😄

[JonHanna]

Looking at the results at first I thought perhaps it was forcing more GC rather than allowing sooner GC. Seeing it's the latter, this seems like good stuff. I like the unionwith added, too. Nothing seems awry, so it's all down to the perf results really, so LGTM.

[karelz]

@jamesqo This can be un-marked as no merge.

Done

[VSadov]

LGTM