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

Fix a bug where calling NextIterator on a "done" list iterator caused it to seemingly switch back to "not done" status #4232

Merged
merged 4 commits into from
Jan 27, 2021

Conversation

wilfwilson
Copy link
Member

@wilfwilson wilfwilson commented Jan 27, 2021

This resolves #4226, reported by @james-d-mitchell. See the discussion there for further context.

Currently, iterators behave differently for immutable dense lists as opposed to other lists w.r.t. whether NextIterator throws an informative error when called on a finished iterator. Moreover, in one of the cases, calling NextIterator on a finished iterator unnecessarily broke the value of IsDoneIterator.

I tried making the error-checking behaviour consistent, but this led to an unacceptable 10% slowdown. Instead I've just removed the breaking of IsDoneIterator, and found one or two ways to speed up the code (mostly by reducing access to record components).

If speed of iterators is important, then I think these changes are useful. If speed is not important, then perhaps I should reinstate proper error checking in NextIterator_DenseList...

I've added some tests, removed some commented-out code, and rearranged a bit of code too - I'm happy to remove either of the latter two for being off-topic.

Quick experiments:

# Mutable list
gap> for i in [1..10] do iter := Iterator([1 .. 10000000]);; for i in iter do od; od; time;
20926 # Before this PR
17429 # After this PR

# Immutable list
gap> for i in [1..10] do iter := Iterator(Immutable([1 .. 10000000]));; for i in iter do od; od; time;
13205 # Before this PR
13158 # After this PR

@wilfwilson wilfwilson added topic: library topic: error handling release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jan 27, 2021
@wilfwilson wilfwilson changed the title Test IsDoneIterator in NextIterator for lists Improve iterators for lists Jan 27, 2021
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you, this PR is exemplary -- nicely separated clean commits make it a breeze to review. 5/5, would review again ;-)

@fingolfin fingolfin merged commit 2bb044b into gap-system:master Jan 27, 2021
@wilfwilson wilfwilson deleted the iterator_list_check_is_done branch January 28, 2021 09:07
@ThomasBreuer ThomasBreuer self-assigned this Feb 16, 2021
@ThomasBreuer ThomasBreuer changed the title Improve iterators for lists Iterators for lists: fixed IsDoneIterator, improved the performance Feb 16, 2021
@ThomasBreuer ThomasBreuer added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 16, 2021
@ThomasBreuer ThomasBreuer removed their assignment Feb 16, 2021
@fingolfin fingolfin changed the title Iterators for lists: fixed IsDoneIterator, improved the performance Fix IsDoneIterator and improve the performance of list iterators Aug 17, 2022
@fingolfin fingolfin changed the title Fix IsDoneIterator and improve the performance of list iterators Fix a bug where calling NextIterator on a "done" list iterator caused it to seemingly switch back to "not done" status Aug 17, 2022
@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: error handling topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible bug in IteratorList
3 participants