Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flatten consecutive chaining of Concat & Prepend #319

Merged
merged 11 commits into from
Jun 30, 2017

Conversation

atifaziz
Copy link
Member

This PR optimises Prepend and Concat such that if they are chained to the same operation or either operation in succession then there's a flat (as opposed to nested today) iteration that takes place.

Before this PR, calling xs.Prepend(2).Prepend(1).Concat(10) will create 3 distinct enumerables, which when iterated will make nested (MoveNext) calls into each iterator. Done in a loop (e.g. xs = xs.Concat(x); and the iteration performance will explode. With this PR, there will be a single enumerable resulting across all consecutive calls and the final iteration will be flat (a single call through MoveNext).

@fsateler
Copy link
Member

Before this PR, calling xs.Prepend(2).Prepend(1).Concat(10) will create 3 distinct enumerables, which when iterated will make nested (MoveNext) calls into each iterator. Done in a loop (e.g. xs = xs.Concat(x); and the iteration performance will explode. With this PR, there will be a single enumerable resulting across all consecutive calls and the final iteration will be flat (a single call through MoveNext).

I like the idea, but the implementation is problematic due to the shared buffer:

var firstSequence = new [] { 1, }.Concat(2);
var second = firstSequence.Concat(3);
var third = firstSequence.Concat(4);

third.Dump(); // prints 1, 2, 4
second.Dump(); // also prints 1, 2, 4

Usually LINQ operators are immutable, in that applying an additional operator does not alter the source sequence, and this PR breaks this expectation.

@fsateler
Copy link
Member

Using ImmutableList would fix the issue, but that would add a dependency.

@atifaziz
Copy link
Member Author

@fsateler That's a great catch! Pity that the tests didn't pick it up. Goes to show you'll always miss something so thanks for the review.

BTW, third.Dump() yields [1, 2, 3, 4].

@atifaziz
Copy link
Member Author

@fsateler I have added tests and fixed the sharing bug. The implementation may be suboptimal, especially for concatenations, but that can be tweaked and improved with time (unless you have some suggestions right away).

@atifaziz atifaziz requested a review from fsateler June 16, 2017 12:37
@atifaziz atifaziz added this to the 2.6.0 milestone Jun 16, 2017
@atifaziz
Copy link
Member Author

@fsateler Are you happy with this now?

@atifaziz
Copy link
Member Author

@fsateler I want to get this into the June release that I was planning for today. I'm assuming you haven't found the time to finish your review but I'll go ahead and merge nonetheless and if you see a glaring issue later then it can always be addressed in a 2.6 patch version.

@atifaziz atifaziz merged commit 7d09ea3 into morelinq:master Jun 30, 2017
@atifaziz atifaziz deleted the FlatPrependConcatChain branch June 30, 2017 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants