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

Use Sequence instead of List in Floodgate #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spacekitteh
Copy link
Contributor

So that we can snoc rather than consing then reversing. This does, however, introduce a limit to the number of actions that can be held (specifically, maxbound::Int.)

So that we can `snoc` rather than consing then reversing. This does, however, introduce a limit to the number of actions that can be held (specifically, `maxbound::Int`.)
@tek
Copy link
Member

tek commented Nov 22, 2022

seems reasonable, but I haven't ever used this. anyone else have opinions?

@KingoftheHomeless
Copy link
Collaborator

KingoftheHomeless commented Nov 22, 2022

Iiiiiiiii think the original intent of Floodgate was that Release resets the state to be empty. Now, it clearly doesn't, and I don't know why it's like that. Floodgate was @isovector 's experiment, and he'd have to comment on that.

Performance-wise, if it's intentional that Release doesn't reset the state to be empty, then yeah, Seq is better. However, if we'd change Release to reset the state, then if I remember how various forms of queues compare to each other correctly, the list solution in this case would actually be more performant than using Seq, when considering amortized performance. The downside of course is that the reverse still means that when a Release is executed there's a delay proportional to the length of the list before the held actions start getting executed. Though I doubt that matters.

In conclusion, either way, a change is warranted here. If a Release means that the floodgate should be emptied and the current implementation is wrong, then we should fix that, and once that's done the question of Seq vs. list gets kind of inconsequential, so we might as well keep the list-based implementation. If a Release means the floodgate shouldn't be emptied and the current implementation is correct, then Seq should be used instead.

@tek
Copy link
Member

tek commented Nov 22, 2022

right. strange

@isovector
Copy link
Member

yeah this is suss. reading through it it should definitely reset the state on Release.

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.

4 participants