Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Small perf tweaks for Select#11841

Merged
VSadov merged 4 commits intodotnet:masterfrom
jamesqo:select-tweaks
Oct 8, 2016
Merged

Small perf tweaks for Select#11841
VSadov merged 4 commits intodotnet:masterfrom
jamesqo:select-tweaks

Conversation

@jamesqo
Copy link
Copy Markdown
Contributor

@jamesqo jamesqo commented Sep 17, 2016

  • If the input is an empty array, we know that it's size will never change (even if it is mutated after the Select call) so we can return a cached result.
    • This allows us to delete some code in ToArray, TryGetFirst and TryGetLast for specially handling empty arrays.
  • _state - 1 can be used as the index into the array instead of a separate _index field. This leaves only 2 pointer-sized fields in SelectArrayIterator.
  • When getting the Count() a SelectEnumerableIterator, we don't have to run the selector on each element of the sequence.

cc @stephentoub @JonHanna @VSadov

@svick
Copy link
Copy Markdown
Member

svick commented Sep 18, 2016

When getting the Count() a SelectEnumerableIterator, we don't have to run the selector on each element of the sequence.

Isn't this a breaking change? For example, consider this code:

IEnumerable<string> GetStrings()
{
    yield return "not int";
}GetStrings().Select(int.Parse).Count();

With the current implementation, this code throws FormatException and I can imagine someone relying on that. But with the new implementation, it wouldn't throw, right?

Though this was already changed for arrays (e.g. new[] { "not int" }.Select(int.Parse).Count()) in #5777, which shipped in 1.0.0. Here is a gist showing the difference between .Net Core and .Net Framework.

public override bool MoveNext()
{
if (_state == 1 && _index < _source.Length)
if (_state == 0 | _state == _source.Length + 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't || be used here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used | to be consistent with other parts of the LINQ codebase where bitwise instead of logical operators are being used, e.g. here. Since it will be very rare for _state to be 0 (only happens when someone casts the result of Select to an IEnumerator) this will save us a branch.

@jamesqo
Copy link
Copy Markdown
Contributor Author

jamesqo commented Sep 18, 2016

@svick Yeah, it could be considered breaking if the input is lazy and the selector has side effects, such as throwing an exception/mutating state. As you pointed out, though, this has already been changed for arrays/lists/other LINQ iterators so I thought we might as well do it here too.

If someone wants to force evaluation of the selector then they are more likely to use a foreach loop in the first place, which is more readable.

@jamesqo
Copy link
Copy Markdown
Contributor Author

jamesqo commented Sep 18, 2016

Test Innerloop Ubuntu14.04 Release Build and Test

@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions area-System.Linq labels Sep 26, 2016
@VSadov
Copy link
Copy Markdown
Member

VSadov commented Oct 8, 2016

LGTM

@VSadov VSadov merged commit 9c784ce into dotnet:master Oct 8, 2016
@jamesqo jamesqo deleted the select-tweaks branch October 8, 2016 00:35
@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Linq enhancement Product code improvement that does NOT require public API changes/additions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants