fix #14: add windowPut--i.e. put with strategy based on num pending puts #17
Conversation
…d on num pending puts
|
|
|
Thanks for working on this! I'll try to look at this sometime this week. Ping if that doesn't happen. |
src/Effect/AVar.purs
Outdated
| -- Drop the head, and push onto the tail | ||
| DropHead e -> do | ||
| void $ Fn.runFn3 _tailPuts ffiUtil e avar | ||
| Fn.runFn4 _snocVar ffiUtil value avar cb |
There was a problem hiding this comment.
Should these operations be atomic? Right now tailPuts/initPuts invokes a callback, which may end up doing something that affects the internal AVar state.
There was a problem hiding this comment.
yes, perhaps there is a need to create atomic versions of these combinations where the callbacks corresponding to dropped entries are invoked after the avar has been updated. I'll submit an update.
It is still possible for the callbacks to invalidate the property being enforced by the strategy. Strategies can still be coded to recover from such deviations.
|
Could you please review the latest of this pull request? Thanks. |
| @@ -50,7 +64,59 @@ kill err avar = Fn.runFn3 _killVar ffiUtil err avar | |||
| -- | the AVar becomes available. Returns an effect which will remove the | |||
| -- | callback from the pending queue. | |||
| put ∷ ∀ a. a → AVar a → AVarCallback Unit → Effect (Effect Unit) | |||
| put value avar cb = Fn.runFn4 _putVar ffiUtil value avar cb | |||
| put = windowPut (const PushTail) | |||
There was a problem hiding this comment.
Since this is by far the most common, maybe it makes sense to avoid the dispatch overhead and call snocVar directly.
There was a problem hiding this comment.
I would be less concerned about this if we didn't have an ADT.
| | DropHead Error -- Drop the head, and push onto the tail | ||
| | DropTail Error -- Drop the tail, and push onto the head | ||
| | SwapHead Error -- Replace the head | ||
| | SwapTail Error -- Replace the tail |
There was a problem hiding this comment.
Is having an actual data type necessary? The upside is that you can pattern match and change the operations later (I don't have a use case for this). The downside is that there is a linear dispatching overhead in the windowPut interpreter. Can you think of a use case that takes advantage of a primitive ADT rather than just bundling the logic up in a closure for each strategy?
| foreign import _tryPutVar ∷ ∀ a. Fn.Fn3 FFIUtil a (AVar a) (Effect Boolean) | ||
| foreign import _takeVar ∷ ∀ a. Fn.Fn3 FFIUtil (AVar a) (AVarCallback a) (Effect (Effect Unit)) | ||
| foreign import _tryTakeVar ∷ ∀ a. Fn.Fn2 FFIUtil (AVar a) (Effect (Maybe a)) | ||
| foreign import _readVar ∷ ∀ a. Fn.Fn3 FFIUtil (AVar a) (AVarCallback a) (Effect (Effect Unit)) | ||
| foreign import _tryReadVar ∷ ∀ a. Fn.Fn2 FFIUtil (AVar a) (Effect (Maybe a)) | ||
| foreign import _status ∷ ∀ a. Fn.Fn2 FFIUtil (AVar a) (Effect (AVarStatus a)) | ||
| foreign import _pendingPuts :: ∀ a. AVar a -> Int |
There was a problem hiding this comment.
This isn't a pure operation since an avar size is mutable. This can potentially cause issues depending on where and how the actual effects are interleaved.
| mPut <- Fn.runFn2 _initPuts ffiUtil avar | ||
| canceller <- Fn.runFn4 _snocVar ffiUtil value avar cb | ||
| canceller <$ do | ||
| traverse_ <@> mPut $ \{cb: cb'} -> cb' (Left e) |
There was a problem hiding this comment.
If we didn't have an ADT, then I think these could easily be implemented in the FFI and avoid the effect thunk and Maybe allocation. What do you think?
I'm not a big fan of the canceller <$ do traverse_ <@> pattern you are using. It took me a second to figure out what it's actually doing. I would suggest:
for_ mPut \{ cb: cb'} -> cb' (Left e)
pure cancellerThis will optimize better.
| eq "barfoo" <$> Ref.read ref | ||
|
|
||
| test_max2_put_take ∷ Effect Unit | ||
| test_max2_put_take = test "max2: put/take" do |
There was a problem hiding this comment.
What is the motivation for this test that's different from max1?
| eq "barfoo" <$> Ref.read ref | ||
|
|
||
| test_window2_put_take ∷ Effect Unit | ||
| test_window2_put_take = test "win2: put/take" do |
There was a problem hiding this comment.
Likewise with max2, it's not clear to me what this is testing vs the first one.
| test_max1_put_put_put_take | ||
| test_window1_put_take | ||
| test_window2_put_take | ||
| test_window1_put_put_put_take_take |
There was a problem hiding this comment.
In general, it seems like we should test all the strategies on some level.
|
I would like to get feedback from @kritzcreek and @garyb on what they think of this API, since they worked on |
please note that the window strategy is based on the number of pending puts. Also note that the
DropHeadstrategy drops the head of the pending puts, but doesn't touch the current value of the AVar. In other words, if DropHead is used to achieve a sliding window, and if the first take/read is called after more than window-width number of puts, the effects of the very first put cannot be dropped/undone. The first put takes effect eagerly, and I have not changed that behaviour.