Many optimizations for System.Linq.Enumerable. - #2318
Conversation
There was a problem hiding this comment.
This will now blow up if _enumerator is null. It should really be:
if (_enumerator != null)
_enumerator.Dispose();Note that no cast at all is needed.
There was a problem hiding this comment.
I added a check for null before disposing the enumerator, but not casting to IDisposable may cause backwards compatibility issues if it has an explicit interface implementation.
There was a problem hiding this comment.
Can you provide an example for this case where it would cause an issue?
jamesqo
changed the title
There was a problem hiding this comment.
nit: would be nice to follow general CoreFX repo convention of using string over String here and change these to double.IsNaN instead (like you used above).
There was a problem hiding this comment.
@jasonwilliams200OK Since I reverted all my changes to Min and Max, the notation has gone back to System.Single.NaN and System.Double.NaN, from the original implementer.
|
cc: @VSadov |
There was a problem hiding this comment.
I'm still unclear what case you're trying to protect against by explicitly casting the IEnumerator<T> to IDisposable<T>. Can you provide an example? This change is also no longer calling Dispose on the base in the case where MoveNext() was never called to initialize _enumerator.
There was a problem hiding this comment.
@stephentoub Fixed this in the latest commit; the cast is no longer there, and base.Dispose() has moved outside the block. I was keeping the cast before because I had seen it in other places throughout the BCL and there seem to be a few questions specifically about this, but I wasn't able to find any problems.
This was no doubt a hang-over of somebody being used to the fact that It seems the compiler already does the smart thing of producing the IL that would come from: So the change doesn't help performance, but I do think changing from the former to the latter is a nice readability improvement. |
|
@hackcraft Saw your message right after I pushed the commit. I think the original writers may have been watching out for an explicit interface implementation or something of the sort that may have required them to explicitly cast to This confused me for a while as well, because there are many questions on Stack Overflow asking about explicit casting to |
|
Hm, it seems we're getting build errors because some of the tests for Got it. The removal of the null checks has affected the count of numbers in the sequence, for nullable numbers. The branch should build (and be ready) after the next commit. |
|
@stephentoub I've undid my changes to |
|
It looks like I'm getting a different error this time- it's coming from Windows builds instead of Linux, and it's something about |
Hmm, that's not what I'm seeing (maybe the tests got rerun between now and then). Both the debug and release tests failed for System.Linq.Tests with ~30 failures: |
|
@dotnet-bot test this please |
|
Splitting this up into a few new PRs. |
This change also re-enables a disabled NameResolution PAL test. Fixes dotnet#2318.
This change also re-enables a disabled NameResolution PAL test. Fixes dotnet/corefx#2318. Commit migrated from dotnet/corefx@cf92d01
7 participants
Just sat down for 5 hours looking at every method in
Enumerable.csto make every optimization I possibly could.Here's a summary of the changes:
WhereEnumerableIterator<TSource>andWhereSelectEnumerableIterator<TSource>no longer check ifIEnumerator<T>implementsIDisposable(it does)SkipWhileIterator: remove redundant checks onyieldingRangeIteratorhas been optimized to avoid double addingint?andlong?and etc. before callingGetValueOrDefault()(you can call a method onnullif it's a nullable)MinandMaxcall:int?andlong?avoid many null-checks after "scrolling forward" to the first non-null element in the collection.float?anddouble?avoid another null-check with a trick usingGetValueOrDefault()