-
Notifications
You must be signed in to change notification settings - Fork 5k
Use Count in Enumerable.Any if available #40377
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The perf losses seem low, particularly when considered long-term (zero allocation means less GC noise, so the few ns spent in the if might still be better).
Perhaps it's also better to let the compiler further optimize things for people?
public static bool Any<T>(this ICollection<T> source);
public static int Count<T>(this ICollection<T> source);
...
?
|
Ok, thanks for weighing in. |
The usages that went down to zero alloc, generally also improved in runtime perf (within the microbenchmark results shared). However, I don't know how heavily Iterators usage is, so the small regression seems fine. Edit: I just realized Iterator is just an
In that case, why not go all in and implement |
Because that makes Any O(N) instead of O(1) when none of the interface are implemented, a common case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ah, right, missed the iterator loop at the end (one iteration for any vs n for count). |
We've been hesitant to make this change in the past, as it adds several interface checks. However, wide-spread "wisdom" is that `Any()` is as fast or faster than `Count() > 0`, and there are even FxCop rules/analyzers that warn about using the latter instead of the former, but in its current form that can frequently be incorrect: if the source does implement `ICollection<T>`, generally its `Count` is O(1) and allocation-free, whereas `Any()` will almost always end up allocating an enumerator. On balance, it seems better to just have `Any()` map closely to `Count()` so that their performance can be reasoned about in parallel.
76097ee
to
f47fce3
Compare
I've fixed that in dotnet/BenchmarkDotNet#1228 |
Thanks, @adamsitnik. |
* Use Count in Enumerable.Any if available We've been hesitant to make this change in the past, as it adds several interface checks. However, wide-spread "wisdom" is that `Any()` is as fast or faster than `Count() > 0`, and there are even FxCop rules/analyzers that warn about using the latter instead of the former, but in its current form that can frequently be incorrect: if the source does implement `ICollection<T>`, generally its `Count` is O(1) and allocation-free, whereas `Any()` will almost always end up allocating an enumerator. On balance, it seems better to just have `Any()` map closely to `Count()` so that their performance can be reasoned about in parallel. * Add test coverage for Enumerable.Any Commit migrated from dotnet/corefx@9021bc1
We've been hesitant to make this change in the past, as it adds several interface checks which do show up in microbenchmarks (as is evidenced below).
However, wide-spread "wisdom" is that
Any()
is as fast or faster thanCount() > 0
, and there are even FxCop rules/analyzers that warn about using the latter instead of the former, but in its current form that can frequently be incorrect: if the source does implementICollection<T>
, generally itsCount
is O(1) and allocation-free, whereasAny()
will almost always end up allocating an enumerator.On balance, it seems better to just have
Any()
map closely toCount()
so that their performance can be reasoned about in parallel. I'd like a second opinion, though. @cston? @ahsonkhan? @bartonjs?produces:
ps @adamsitnik, I could not figure out how to get the benchmark to take an
IEnumerable<int>
; everything I tried resulted in errors likeerror CS0266: Cannot implicitly convert type 'object' to 'System.Collections.Generic.IEnumerable<int>'
. This is with benchmarkdotnet 11.5.