Specialize the single-selector overload of SelectMany. by jamesqo · Pull Request #13942 · dotnet/corefx

jamesqo · 2016-11-24T05:09:53Z

e.SelectMany(i => i) is a very popular option for flattening a list of enumerables: http://stackoverflow.com/questions/1590723/flatten-list-in-linq

This PR optimizes SelectMany calls followed by ToArray or ToList, leading to a substantial (~40%) speedup. Instead of looping through each item of the projected sequence, yield returning it from the iterator, and adding it to the list/LargeArrayBuilder, we simply call AddRange.

Performance test: https://github.com/jamesqo/Dotnet/blob/05daf42403b615942706a7e9be32c8b22db80e67/Program.cs Results: https://gist.github.com/jamesqo/635e6e49b3ceb6e9527161f3472188f7

You may notice that there are a lot of regressions (substantially more allocations happening) for ToList. That puzzled me too, until I found out the version of S.P.CoreLib I was using did not include this change; in my tests, a buffer was being allocated every time List.AddRange was called. Still, however, the new implementation was faster.

I've also added more testing for these changes in the PR.

cc @JonHanna @VSadov @stephentoub

jamesqo · 2016-11-24T05:10:41Z

src/System.Linq/src/System/Linq/SelectMany.cs

I would have preferred to keep this SelectManyIterator, but the compiler complains because there are other SelectManyIterator members.

jamesqo · 2016-11-24T05:11:36Z

src/System.Linq/src/System/Linq/SelectMany.cs

TODO: Add tests similar to the ones in Concat to make sure checked is used.

Are you going to add these tests in this PR?

Ah, I forgot. Yes.

stephentoub · 2016-11-24T12:22:05Z

src/System.Linq/src/System/Linq/SelectMany.cs

Should we set _subEnumerator to null here? That would help in the case where the subsequent GetEnumerator() call throws, at which point if we didn't null this out we'd be holding on to a disposed enumerator.

Sure, done.

stephentoub · 2016-11-24T12:32:58Z

You may notice that there are a lot of regressions (substantially more allocations happening) for ToList. That puzzled me too, until I found out the version of S.P.CoreLib I was using did not include this change; in my tests, a buffer was being allocated every time List.AddRange was called

What about the regressions for iteration and ToArray?

stephentoub

Other than my few questions/comments, LGTM. Thanks!

jamesqo · 2016-11-24T15:54:07Z

@stephentoub

What about the regressions for iteration and ToArray?

I believe those are just random fluctuations in the results... most of them appear to be when the sub-collection length is really small, like 1-3, and AddRange doesn't help much. Also this PR is only supposed to help ToArray / ToList perf, I only included iteration benchmarks to show they maintained the status quo.

edit: I forgot to add, most of the regressions are like 5-10% and inconsistent.

VSadov · 2016-11-24T20:25:31Z

LGTM
Nice change!!

Also was interesting to learn about #6892. Nice change too. Normally I would not like an idea of exposing internal buffers to strangers, but indeed, List<T> is mutable and hardly a lot of extra damage could be done from charing in that case.

stephentoub · 2016-11-24T21:05:05Z

I believe those are just random fluctuations in the results

I'm not convinced that there aren't real regressions in some cases. Consider the common case where the enumerable returned for each iteration isn't an ICollection. Won't List.AddRange with its current implementation end up doing more work than if each element were Add'd individually?

JonHanna · 2016-11-24T21:01:11Z

src/System.Linq/src/System/Linq/SelectMany.cs

_subEnumerator may not be null here, and need to be disposed too, if the enumeration is jumped out of.

Good point. I'll add a dispose for that too.

jamesqo · 2016-11-25T00:18:25Z

@stephentoub Ah, you make a good point. Looks like how it's currently implemented we'll end up in a loop calling Insert on each item on the source, while right now we call Add on each item in the iterator. Insert has 2 more branches than Add; it's debatable whether adding 2 extra branches is faster than 1 virtual method call, but at any rate it looks like the code in List can be improved. I'll make a PR to coreclr.

stephentoub · 2016-11-26T12:39:16Z

src/System.Linq/src/System/Linq/SelectMany.cs

I believe that if previously an exception came out of here, such as from the MoveNext, the instance would be Dispose'd immediately; now it requires Dispose to be called on the enumerator explicitly. Are we ok with that @VSadov?

Doesn't this happen every time we replace a yield-based iterator with a class?

@VSadov, can you comment on whether you think this is an acceptable change when you have time? Thanks.

It is a common practice in LINQ. See, for example implementation of SelectEnumerableIterator
Since client can stop enumerating at any time, voluntarily or due to a fault, the client needs to be able to dispose the entire internal state of the iterator. Thus we create a composite Dispose.
We could stop just at that, but we often, as a good effort, dispose on state transitions as well. That is mostly to release resources early and does not need to be hardened for faulting scenarios.

jamesqo · 2016-12-05T23:00:43Z

Resolved merge conflicts

jamesqo · 2016-12-05T23:48:53Z

@stephentoub Responding to your comments from earlier:

I'm not convinced that there aren't real regressions in some cases. Consider the common case where the enumerable returned for each iteration isn't an ICollection.

My tests didn't actually hit that branch since each subcollection was an array. However, I modified them slightly to remove the ToArray call here and ran with a build of coreclr that included dotnet/coreclr#8306 (which has been merged). It still looks like mostly improvements excepting a couple of fluctuations: https://gist.github.com/jamesqo/fb78ba5b5b3c26f4a67fac4aa40dabf3

VSadov · 2016-12-28T00:24:30Z

LGTM

Specialize the single-selector overload of SelectMany. Commit migrated from dotnet/corefx@5bf69d1

dnfclas added the cla-already-signed label Nov 24, 2016

jamesqo commented Nov 24, 2016

View reviewed changes

stephentoub reviewed Nov 24, 2016

View reviewed changes

karelz added the area-System.Linq label Nov 24, 2016

karelz assigned jamesqo, VSadov and OmarTawfik Nov 24, 2016

JonHanna suggested changes Nov 24, 2016

View reviewed changes

jamesqo mentioned this pull request Nov 25, 2016

Better bulk adds for List. dotnet/coreclr#8306

Merged

stephentoub reviewed Nov 26, 2016

View reviewed changes

jamesqo added 7 commits December 5, 2016 17:56

Optimize common overload of SelectMany.

4a189f1

Add convoluted tests that worked on the first try.

46e3307

Enable the new implementation!

17f4a88

Renaming.

46139ba

Respond to PR feedback.

f22f80c

Add tests to make sure SelectMany.Count throws on overflow.

48d6e68

Dispose _subEnumerator too.

ac1f329

jamesqo force-pushed the select-many branch from 8fda260 to ac1f329 Compare December 5, 2016 22:58

Fix indentation.

2ed5325

karelz modified the milestone: 1.2.0 Dec 6, 2016

jamesqo mentioned this pull request Dec 22, 2016

Reduce allocations in {Concat,SelectMany,Append,Prepend}.ToArray by close to 50% #14675

Merged

VSadov merged commit 5bf69d1 into dotnet:master Dec 28, 2016

jamesqo deleted the select-many branch December 28, 2016 01:29

manofstick mentioned this pull request Jan 14, 2019

[WIP ] System.Linq optimisations via architecture change #34208

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#13942 from jamesqo/select-many

ab25b8f

Specialize the single-selector overload of SelectMany. Commit migrated from dotnet/corefx@5bf69d1

Conversation

jamesqo commented Nov 24, 2016

Uh oh!

jamesqo Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Nov 24, 2016

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

jamesqo commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Nov 24, 2016

Uh oh!

stephentoub commented Nov 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesqo commented Nov 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesqo Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesqo commented Dec 5, 2016

Uh oh!

jamesqo commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Dec 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jamesqo Nov 24, 2016 •

edited

Loading

jamesqo commented Nov 24, 2016 •

edited

Loading

jamesqo Dec 20, 2016 •

edited

Loading

VSadov Dec 28, 2016 •

edited

Loading

jamesqo commented Dec 5, 2016 •

edited

Loading