Optimize Where{.Select}.To{Array,List} & Count.

[jamesqo]

Changes:

• Refactor Count such that the part that checks for internal Linq interfaces is segregated from the part that checks for the rest of the interfaces.
  • The internal Count method is probably only going to be used by Linq, so I put this in a new file & made EnumerableHelpers partial, so other assemblies using that class don't have to drag in extra IL.
  • Put a chunk of Count in a non-generic method, so we save generating a substantial chunk of code for every different generic instantation of the method.
• Implement IIListProvider on all of the Where / Where.Select iterators, substantially speeding up all ToArray / ToList operations by avoiding virtual calls / field stores.
• Remove an int field from some Where iterators by reusing _state - 1 as the index.

@stephentoub, @VSadov

[jamesqo]

Note: Typecast order has changed slightly, we now check IIListProvider<T> first instead of ICollection<T>. Not sure if that matters.

[karelz]

@jamesqo what is left? Just code review? cc: @VSadov

[jamesqo]

@karelz It would be preferable if #13076 could be reviewed and merged first, otherwise I'd have to make another PR to change some of the logic here again.

[karelz]

The dependency is merged, can we move forward on this one @jamesqo?

[jamesqo]

@karelz, yes, we can. I just broke my main laptop yesterday, however, so it may be a day or two before I get to finishing this up.

[jamesqo]

Alright, finally finished working on this. @stephentoub, @VSadov please review.

[jamesqo]

Perf results: here. Count has (unsurprisingly) regressed a little since we're adding 3 new method calls, ToArray and ToList have both improved significantly.

[jamesqo]

cc @JonHanna

[stephentoub]

What is the value of this change?

[stephentoub]

You've been doing this kind of separation in a few places. Is it really worthwhile? This adds cost to the (common) case of using Count() on a regular 'ol enumerable. I don't have numbers, but I'd venture to guess this is the most common case, yet we've made it the most expensive now.

[jamesqo]

@stephentoub I did so because I figured 1 or 2 extra procedure calls wouldn't be much compared to the overhead of repeatedly calling MoveNext, a bunch of typecasts beforehand, GetEnumerator / Dispose, etc. I did a small benchmark here testing for regression; surprisingly, the difference does not seem to be measurable even for the length 1 case.

but I'd venture to guess this is the most common case,

We only go down this path if the enumerable is lazy (we can't determine its size beforehand) & does not come from/is not optimized to implement IIListProvider by Linq, e.g. a method that uses yield return. Is this the most common case?

[jamesqo]

@stephentoub It should also be noted that where we detract we add: Separating this part out into a new method saves the JIT from generating a sizeable chunk of code for each generic instantiation of Count. I can post some numbers if you'd like.

[stephentoub]

We only go down this path if the enumerable is lazy

This is extremely common. If you have a concrete T[], you're much better off calling Length. If you have a List<T> or an ICollection<T> or an IList<T>, etc., you're better off just using its Count property. It's only when you have one of those that's typed as IEnumerable<T> that you'd use Count(), whereas Count() is the only option for enumerables produced by LINQ, by iterators, etc.

Separating this part out into a new method saves the JIT from generating a sizeable chunk of code for each generic instantiation of Count

I understand. But at the same time, we're not going to go through every generic method in the platform and pull out groups of lines here and there that aren't dependent on the generic parameters and separate them into their own methods. That would be overkill. What makes these few lines special?

[stephentoub]

I can post some numbers if you'd like.

Do you have example libraries or apps in mind, where Count() is used on an IEnumerable<T> with a value type T in cases where this code would not be hit? This only ends up saving on generated assembly if this code is never hit for a given value type T (or any reference type) but the earlier part of the method is. If that's really common, then sure, I can see it making sense... I'm just skeptical.

[jamesqo]

@stephentoub

This only ends up saving on generated assembly if this code is never hit for a given value type T (or any reference type) but the earlier part of the method is.

Actually, I believe this saves generated assembly in 2 scenarios:

1. Like you mentioned, if for a particular value type T the collection always implements one of the interfaces and does not hit this codepath, then this will save.
2. Another scenario is if this codepath is hit twice by two different value types, it will only be generated once, since it's non-generic. Example

lazyBytes.Count<byte>();
lazyInts.Count<int>();

Do you have example libraries or apps in mind,

@stephentoub, I have been trying to measure the impact this has on Roslyn for a couple of days. However, I'm running into some runtime issues using Roslyn with a custom build of coreclr. If you're still not convinced by this argument, I can remove this part of the PR out for now, and make a new PR when I get numbers. Will separate into new PR for now

[stephentoub]

Why is the AsEnumerable() necessary? If it's purely to satisfy the compiler's need for the if/else branches to have a common type, I'd much prefer to see a cast used.

[stephentoub]

Nit: could just be:

foreach (TSource item in _source)

[jamesqo]

Will do

[stephentoub]

Same nit

[stephentoub]

Same nit

[jamesqo]

For Lists, using foreach will be slower since the compiler doesn't optimize that.

[stephentoub]

Same nit

[stephentoub]

Same nit... I'll stop commenting on these.

[stephentoub]

What's the reason for calling Count rather than open coding this, which would avoid the cost of invoking the selector for every element that passes the predicate? Are we concerned about not throwing exceptions where we previously threw them?

[jamesqo]

@stephentoub, I wasn't sure that GetCount was worth optimizing; e.Count(pred) is typically preferred over e.Where(pred).Count(), and optimizing this function would involve some imperative code (maintaining an int variable we would increment every time a predicate was hit).

Thinking about it more though, someone may pass e.Where(...) to a function that invokes Count() on the enumerable, so I guess writing this inline may be worthwhile.

[stephentoub]

Rather than calling EnumerableHelpers.Count, couldn't we just iterate ourselves here, which would avoid the enumerator allocation, the interface calls, etc.? Same for the WhereListIterator case.

[stephentoub]

@VSadov, are you ok with the small change here where previously we'd execute the selector function and now we don't? This is good for perf, but it does mean that if the selector would have thrown an exception, now such an exception won't happen.

[jamesqo]

@stephentoub For reference, a similar change was made in #11841.

[VSadov]

I think we must run the selectors. It is reasonable for the user that selectors will run.
In fact, it does not seem very unlikely that user may do ".Count" specifically just to run sideeffecting selectors.

---

In reply to: 88807476 [](ancestors = 88807476)

[VSadov]

We should revisit the change in #11841. I think we may have gone too far.
It seems ok to use GetCount(onlyIfCheap) to preallocate buffers, but skipping selectors in a context of aggregating operator (even if trivial - like Count) now seems potentially breaking.

---

In reply to: 88977111 [](ancestors = 88977111,88807476)

[stephentoub]

Same question:

index = _state++;

?

[stephentoub]

@VSadov, same question here.

[stephentoub]

@VSadov, same question here.

[VSadov]

Re: can we skip selectors/predicates in this optimization

It is a tough one.

Generally, assuming that anything can have sideeffects is indeed very constraining to what kind of optimizations we can do.
In some cases it is hard to specify how much of the user code runs and in what order. We definitely reserve the rights to substitute iteration for indexing when we find that possible.

After thinking about this for quite a while, I think Count is a kind of aggregating operator. Even though the actual results are not collected, it seems reasonable for the user to expect that they would be computed, and as such he may expect to observe the sideeffects from selectors/predicates etc.

I.E. – I could see someone using a selector that writes to the console, and use Count as a way to run the query for sideeffects only.

I think we should not take the change and check for other changes like this, that we might have accepted in the past. (like: #11841 )

Here is what I think is the root cause -
We do have a method for obtaining counts internally -

GetCount(bool onlyIfCheap)

I think the method should not be used to bypass selectors when actually running the query. It is ok to use it to preallocate internal buffers. That still have an assumption that the “cheap” way of obtaining the underlying “.Count” is idempotent, but it is a kind of assumption that we agreed in the past to be acceptable.

It does not seem to be acceptable to assume that user-supplied selectors/predicates are sideeffects-free in a context of aggregating query.

I.E. –

1. it would be ok to preallocate a buffer based on GetCount(bool onlyIfCheap)
2. it would be ok to compute actual Count via , but only as long as there are no funcs to run
3. if we have selectors/predicates or other funcs, they must run in aggregating queries.

[jamesqo]

@VSadov, what you are saying seems reasonable. It will regress performance for Count, but not scenarios where we want to preallocate a buffer. I've opened #13910 to track reversal of the optimizations, & I will update this PR later to revert the related changes in this file.

[jamesqo]

@stephentoub @VSadov I re-did the EnumerableHelpers.Count refactoring at 70ac0aa which segregates the IIListProvider check from the public interfaces, and have changed the GetCount implementations back to shims:

public int GetCount(bool onlyIfCheap) => onlyIfCheap ? -1 : EnumerableHelpers.GetCount(this);

so the selector is always evaluated. I think this is safe to merge once green.

[VSadov]

equivalent.Count() is evaluated on every iteration. Is that intentional?

[jamesqo]

@VSadov, I figured it probably didn't matter since this is test code. The length of the source that will be fed to the method is only 10.

[VSadov]

LGTM
There is one comment on ".Count" being called more than necessary in one of the tests. Not sure if that is intentional.

[stephentoub]

I know I previously questioned the value of separating this out. However, I see this helper is being used in places like https://github.com/dotnet/corefx/pull/12703/files#diff-0c9ac9d1102269d53d52c52b5cdaff8bR20. From such places, we know that the ICollection interface casts will fail, so if this were separated out, we could just call to this piece directly and avoid those casts.

[jamesqo]

@stephentoub I do not think Count() is worth optimizing for if we have to run the selector on everything anyway. I'm not sure it's worth the extra ugliness to have

public int GetCount(bool onlyIfCheap) => onlyIfCheap ? -1 : EnumerableHelpers.CountAndDispose(GetEnumerator());

Seems like more of an implementation detail to me.

[stephentoub]

I'm not sure it's worth the extra ugliness to have

What extra ugliness? I'm talking about the difference between:

EnumerableHelpers.Count(this);

and:

EnumerableHelpers.CountIterate(this);

or some similar name.

[jamesqo]

@stephentoub Ok. I was trying to say that if we cared about optimizing this function at all, we could have just written the whole thing inline like I did here, and avoid any typecasting. But since we are running the selector anyway, I don't think it's worth trying to optimize these functions; we should not add any additional complexity trying to do so, even if that is just one additional method.

I think this can be revisited in another PR if performance here turns out to matter.

[stephentoub]

But since we are running the selector anyway

There isn't a selector in cases like:
https://github.com/dotnet/corefx/pull/12703/files#diff-e6e91d17f21cf11b8d7b5ba1c23c933aR110

I don't think it's worth trying to optimize these functions

Why?

we could have just written the whole thing inline like I did here

Yes, like I suggested at https://github.com/dotnet/corefx/pull/12703/files#r87900623, which got a thumbs up from you but doesn't appear to have been addressed, so I'm unclear what the plan is.

[jamesqo]

Why?

@stephentoub I usually capture the sequence into an array/list and get its length instead of using Count when I need it, so for me Count isn't that common. But, I've changed my mind since that may be different for other people. I've updated the PR, reverting the last commit.

[jamesqo]

@stephentoub OK. Should be good to merge once green 🎉

[stephentoub]

Nit: var => ?

[stephentoub]

Nit: var => ?