Enumerable improvements [WIP] - #2237
Conversation
confirm number of matches equals 1" to "find match, then confirm there isn't another". Allows for slight performance improvement in 0-match and 1-match case as loop does less work, and great improvement in many- match case as evaluation can short-circuit.
in Min, Max and Average.
don't) and then see if you can find more.
There was a problem hiding this comment.
I'm not sure this change to hide the check-and-throw in a method is an improvement. I agree it helps clean up the code, but in the past I believe I've seen this have implications on inlining, so while this could be a small readability improvement, I think it's possible it could also be a small performance hit. I could be remembering incorrectly, or maybe it's so small in this context that it doesn't matter, but it makes me hesitate.
Regardless, I'd prefer to see such a change separated out into its own PR so that it can be more easily evaluated on its own.
There was a problem hiding this comment.
With small enough methods a reason for doing it can be precisely to encourage inlining (if the result is that the containing method drops below the jitter's threshold). I think you've a good point about it being in a different PR though. I shall revert this particular commit soon.
There was a problem hiding this comment.
I'd actually thought it would make things worse, as I've seen the JIT refuse to inline methods that contain throw, and I thought it would refuse to inline these helpers because of that, thus we'd actually be increasing the number of function calls that couldn't be inlined. But, I just checked and tried it out, and it appears at least the VS2015 x86 and x64 JITs are able to inline methods with throws in some cases, just not in others (e.g. https://github.com/dotnet/coreclr/blob/master/src/jit/importer.cpp#L12588-L12612), and these helpers are actually able to be inlined, at least in my experiments.
In any event, thanks for separating them out. Since we'd be changing such error checking paths, I'd also hope to see tests in place that would exercise all of those checks being changed.
There was a problem hiding this comment.
The 'D' here on the first element is pretty sublte and easy to miss or even accidentally remove, at which point the test changes. How about explicitly typing the array, i.e. new double[] instead of new []?
… reduce" This reverts commit 3309010. Conflicts: src/System.Linq/src/System/Linq/Enumerable.cs
There was a problem hiding this comment.
I do not think former version supported Reset. It is better to not introduce it because in will or will not work depending on underlying source which would be inconsistent and hard to program against. More so, once it is supported it will be hard to take back if future optimizations cannot support Reset.
Generally Linq iterators are not supporting reset. So it is better to not expand the feature surface here.
|
As a general note, I think this would go better in the following order:
|
|
@hackcraft, what do you think of @VSadov's suggested plan of attack here? I think it'd make sense to close out this PR and submit PRs instead for each of his numbered items in the order he recommends (potentially splitting into further PRs if deemed appropriate). Hopefully 1 and 2 would go through without much issue, and then we can have the more difficult conversations around 3. |
|
I was actually planning to submit 3 PRs in parallel for those three cases, but won't look at this until the weekend. |
Tests originally commited as part of pull-request dotnet#2237 moved out into separate pull-request.
And of course, that's a silly idea, because doing them in parallel means either committing code without tests, which is backwards, or else branching the branches for the other two PRs of the branch in #2326 which has its own issues. Let's look at the tests, as per #2326. Then I'll split off the other two aspects of this, and close this PR. |
Tests originally commited as part of pull-request dotnet#2237 moved out into separate pull-request. Consistently favour type names over c# names. Single element sequences on Min and Max (What other edge cases should we hit?) MinValue and MaxValue for element types. Min & Max with selector Remove tests on short-circuit/not short-circuit for now. More skip tests.
Tests originally commited as part of pull-request dotnet#2237 moved out into separate pull-request. Consistently favour type names over c# names. Single element sequences on Min and Max (What other edge cases should we hit?) MinValue and MaxValue for element types. Min & Max with selector Remove tests on short-circuit/not short-circuit for now. More skip tests.
Tests originally commited as part of pull-request dotnet#2237 moved out into separate pull-request. Consistently favour type names over c# names. Single element sequences on Min and Max (What other edge cases should we hit?) MinValue and MaxValue for element types. Min & Max with selector Remove tests on short-circuit/not short-circuit for now. More skip tests.
stephentoub
removed
the
api-needs-work
|
@hackcraft, can this be closed now? Has everything you wanted to save from it been separated out? |
|
@stephentoub There's still a bunch of stuff that I'm separating out from it a batch at a time (expect Min and Max optimisations with tests on performance improvement this weekend) but it can still be closed in the meantime. |
|
Thanks, @hackcraft. |
Tests originally commited as part of pull-request dotnet/corefx#2237 moved out into separate pull-request. Consistently favour type names over c# names. Single element sequences on Min and Max (What other edge cases should we hit?) MinValue and MaxValue for element types. Min & Max with selector Remove tests on short-circuit/not short-circuit for now. More skip tests. Commit migrated from dotnet/corefx@f8faadf
8 participants
Fixes #2238
Changes to System.Linq.Enumerable, focusing on reducing the size of loops and on short-circuiting when an existing test points to a short-circuit opportunity. Also adds tests.