polysemy-research / polysemy-zoo

:monkey::panda_face: Experimental, user-contributed effects and interpreters for polysemy
BSD 3-Clause "New" or "Revised" License
70 stars 20 forks source link

Use Sequence instead of List in Floodgate #80

Open spacekitteh opened 1 year ago

spacekitteh commented 1 year ago

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 commented 1 year ago

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

KingoftheHomeless commented 1 year ago

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 commented 1 year ago

right. strange

isovector commented 1 year ago

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