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

SkipLast is not optimal #626

Open
Orace opened this issue Oct 31, 2019 · 2 comments · May be fixed by #644
Open

SkipLast is not optimal #626

Orace opened this issue Oct 31, 2019 · 2 comments · May be fixed by #644

Comments

@Orace
Copy link
Contributor

Orace commented Oct 31, 2019

SkipLast use CountDown and therefor store all the elements.
We can manage to store only the number of element to skip.
Store the full sequence could and should be avoided.

Orace added a commit to Orace/MoreLINQ that referenced this issue Oct 31, 2019
@Orace Orace closed this as completed Oct 31, 2019
@Orace Orace changed the title SkipLast is not optimal in memory SkipLast is not optimal Oct 31, 2019
@Orace
Copy link
Contributor Author

Orace commented Oct 31, 2019

Anyway the current implementation is not really optimal (intermediate object creation, test for null, etc...)
For performance reason, I think that the MoreLinq methods should not call other MoreLinq methods but go straightforward to the most efficient implementation.

This is a big bite and a long running task.
Anyway this is a start for it.

@atifaziz
Copy link
Member

atifaziz commented Nov 1, 2019

For performance reason, I think that the MoreLinq methods should not call other MoreLinq methods but go straightforward to the most efficient implementation.

Yes, I agree with that. In general, the approach and steps to adding something non-trivial and useful has been:

  1. If it can be implemented in terms of other operations then do that first.
  2. Add unit tests.
  3. Get it working.
  4. Release for field use.
  5. Optimize in a future version (unit tests from 2 keeping us real at this stage), even if it means hand-rolling the implementation.

The benefit of 1 is that we can leverage all the tested operations.

We need to get coverage reporting added to the project so we can make sure tests are sufficiently covering the code before refactoring wildly. Meanwhile it's better to err on the safe side.

Also keep in mind that a lot of the operators in MoreLINQ can be implemented for asynchronous streams and that will explode the implementation base. When you reuse other operators, there will be far less implementation duplication. In the end, we have to strike a balance.

@Orace Orace linked a pull request Nov 5, 2019 that will close this issue
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 a pull request may close this issue.

2 participants