Skip to content

Apply optimizations and fixes to Enumerable.Take(Range) #48634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
eiriktsarpalis merged 14 commits into from
Mar 1, 2021
Merged

Apply optimizations and fixes to Enumerable.Take(Range) #48634

eiriktsarpalis merged 14 commits into from
Mar 1, 2021

Conversation

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Feb 23, 2021 •
edited
Loading

Makes the following changes to the new Enumerable.Take(Range) implementation:

  • Separate SizeOpt and SpeedOpt implementations for non-fromEnd ranges.
  • Optimize fromEnd ranges using Enumerable.TryGetNonEnumeratedCount to obtain the source count.
  • Change enumerator disposal semantics to more closely follow the TakeLast and SkipLast implementations.
  • Finally, expresses the TakeLast and SkipLast in terms of the Take(Range) implementation.

Fix #48631.

@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Feb 23, 2021
@eiriktsarpalis eiriktsarpalis self-assigned this Feb 23, 2021
@ghost
Copy link

ghost commented Feb 23, 2021

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

Makes the following changes to the new Enumerable.Take(Range) implementation:

  • Separate SizeOpt and SpeedOpt implementations for non-fromEnd ranges.
  • Optimize fromEnd ranges using Enumerable.TryGetNonEnumeratedCount to obtain the source count.
  • Change enumerator disposal semantics to more closely follow the TakeLast and SkipLast implementations.
  • Finally, expresses the TakeLast and SkipLast in terms of the Take(Range) implementation.
Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Linq
Milestone: 6.0.0

@eiriktsarpalis eiriktsarpalis mentioned this pull request Feb 23, 2021
Closed
@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Feb 23, 2021

cc @Dixin

@runfoapp runfoapp bot mentioned this pull request Feb 24, 2021
Closed
@eiriktsarpalis eiriktsarpalis force-pushed the enumerable-take-optimizations branch from 8a0a64f to e7d5a43 Compare February 24, 2021 23:01
@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Feb 25, 2021

I've been investigating the seemingly unrelated test failure affecting mono debug builds. The issue is caused by xunit's generic test parameter resolution logic: the removal of the private SkipLastIterator method seems to result in incorrect generic parameters being resolved for certain generic theory parameters.

I have pushed a commit to demonstrate this and to further show that the failures are not due to a regression introduced by this PR, but rather a bug of xunit running over mono debug builds.

@runfoapp runfoapp bot mentioned this pull request Feb 25, 2021
Closed
@eiriktsarpalis
optimize Take(Range) for countable enumerables
f38c0d6
@eiriktsarpalis
Implement TakeLast over Take(Range)
a5339bb
@eiriktsarpalis
Implement SkipLast over Take(Range)
e1bd78c
@eiriktsarpalis
remove unneeded checked increment
3558108
@eiriktsarpalis
fix typo in tests
b89a0de
@eiriktsarpalis
remove ToLookup inputs from NonEnumeratedCount tests
faed507 
work around an xunit issue binding inputs with nested
generics to generic theories.
@eiriktsarpalis
remove leftover code
7dad846
@eiriktsarpalis
Revert "remove ToLookup inputs from NonEnumeratedCount tests"
cd1c805 
This reverts commit 6d160f618c54abb77c602840a5ecd359477468d4.
@eiriktsarpalis
try debugging test failure
9478f52
@eiriktsarpalis
remove ToLookup inputs from NonEnumeratedCount tests
101ad8e 
work around an xunit issue binding inputs with nested
generics to generic theories.
@eiriktsarpalis
Revert "try debugging test failure"
0c06999 
This reverts commit d7b11dab001b82cc533449c7f23e2bb90ec2515d.
@eiriktsarpalis eiriktsarpalis force-pushed the enumerable-take-optimizations branch from 7c73ea3 to 0c06999 Compare February 26, 2021 15:26
@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Feb 26, 2021

Hi @stephentoub @layomia, this is ready for review, would you be able to take a look?

stephentoub
stephentoub reviewed Feb 26, 2021
View reviewed changes
stephentoub
stephentoub reviewed Feb 26, 2021
View reviewed changes
Base automatically changed from master to main March 1, 2021 09:08
@eiriktsarpalis eiriktsarpalis merged commit f7308f0 into dotnet:main Mar 1, 2021
@eiriktsarpalis eiriktsarpalis deleted the enumerable-take-optimizations branch March 1, 2021 13:56
@DrewScoggins DrewScoggins mentioned this pull request Mar 11, 2021
Open
@DrewScoggins DrewScoggins mentioned this pull request Mar 22, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@layomia layomia Awaiting requested review from layomia

Assignees

@eiriktsarpalis eiriktsarpalis

Labels

area-System.Linq

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Optimize the implementation of LINQ partitioning APIs

2 participants

@eiriktsarpalis @stephentoub