Skip to content
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.

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

Optimise FallbackIfEmpty #464

Conversation

julienasp
Copy link
Contributor

@julienasp julienasp commented May 28, 2023

This PR improves the FallbackIfEmpty performance in the case of collections.

Fixes #367

@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (56f77c0) 91.44% compared to head (1879e0c) 91.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   91.44%   91.47%   +0.02%     
==========================================
  Files         245      245              
  Lines        7936     7949      +13     
  Branches     1608     1611       +3     
==========================================
+ Hits         7257     7271      +14     
+ Misses        461      460       -1     
  Partials      218      218              
Impacted Files Coverage Δ
Source/SuperLinq/FallbackIfEmpty.cs 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

Check out this test for an example of why we cannot evaluate collection count when the operator is called.

In further consideration, I wonder if I was forgetting that when I write the issue. In theory, this could be abstracted to an If() that selects the right IEnumerable, but in practice, don't know that it's an actual optimization.

I'm going to leave this open for a few days while I ponder whether to ask you to improve this or to ask me to stop coming up with bad ideas... :)

@viceroypenguin
Copy link
Owner

viceroypenguin commented Jun 8, 2023

@julienasp I think I would like to have FallbackIfEmpty implemented as a CollectionIterator. You can review DoIterator for a minimum implementation and Insert method for example of how it can be checked for two IEnumerable<>. I think the GetEnumerable() and CopyTo() methods can be implemented.

@julienasp
Copy link
Contributor Author

@julienasp I think I would like to have FallbackIfEmpty implemented as a CollectionIterator. You can review DoIterator for a minimum implementation and Insert method for example of how it can be checked for two IEnumerable<>. I think the GetEnumerable() and CopyTo() methods can be implemented.

perfect i'll work on that this weekend

@julienasp julienasp force-pushed the julienasp/2023/optimiseFallbackIfEmpty branch from 0c35eb5 to c38c883 Compare June 17, 2023 16:13
Copy link
Owner

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

Thanks! Looking good so far.

Source/SuperLinq/FallbackIfEmpty.cs Outdated Show resolved Hide resolved
Source/SuperLinq/FallbackIfEmpty.cs Outdated Show resolved Hide resolved
Source/SuperLinq/FallbackIfEmpty.cs Outdated Show resolved Hide resolved
Tests/SuperLinq.Test/FallbackIfEmptyTest.cs Show resolved Hide resolved
Tests/SuperLinq.Test/FallbackIfEmptyTest.cs Outdated Show resolved Hide resolved
Tests/SuperLinq.Test/FallbackIfEmptyTest.cs Outdated Show resolved Hide resolved
Tests/SuperLinq.Test/FallbackIfEmptyTest.cs Show resolved Hide resolved
Copy link
Owner

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

I think this is the last round of changes. 🤞🏼

Source/SuperLinq/FallbackIfEmpty.cs Show resolved Hide resolved
Tests/SuperLinq.Test/FallbackIfEmptyTest.cs Outdated Show resolved Hide resolved
Tests/SuperLinq.Test/FallbackIfEmptyTest.cs Outdated Show resolved Hide resolved
@viceroypenguin viceroypenguin added this to the 5.2.0 milestone Jun 18, 2023
@viceroypenguin viceroypenguin merged commit 89ad40e into viceroypenguin:master Jun 18, 2023
@viceroypenguin
Copy link
Owner

@julienasp Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FallbackIfEmpty Optimizations
2 participants