Initial implementation #1

natefaubion · 2017-07-04T05:52:32Z

Implements a low-level Eff based interface for AVars with an improved API which is consistent with Haskell's MVars.

jdegoes · 2017-07-06T16:18:49Z

src/Control/Monad/Eff/AVar.js

+    if (avar.error !== null) {
+      value = left(avar.error);
+      // Error callback ordering is somewhat undefined, but we try to at least
+      // be fair by interleaving puts and takes.


If the logic of take/put is correct, then this is overgeneralized and supports more possibilities than can actually exist.

data AVarState a = Empty | Full a | Surplus (NonEmptyList (Tuple a (Callback Unit))) | Deficit (NonEmptyList (Callback a)) (List (Callback a)) -- takers, readers

Taking advantage of these constraints could simplify things and remove the need to handle "impossible cases", as well as provide stronger guarantees.

Note that NonEmptyList above should actually be something like NonEmptyFifoQueue.

You are right, the error handling here is overly general. It should be fine to just drain all of them because it isn't possible to have multiple producers and consumers at the same time.

jdegoes · 2017-07-06T16:22:59Z

src/Control/Monad/Eff/AVar.js

+
+    // Callbacks could have queued up more items so we need to guard on the
+    // actual mutable properties.
+    if (avar.value === EMPTY && ps.size === 0 || avar.value !== EMPTY && ts.size === 0) {


Seems you could also model changes to state as AVarState a -> AVarState a, such that only drainVar would actually compute state, the rest would just compose new state updaters together. This handles the reentrant problem.

I will have a think about this, and perhaps write up a simple benchmark.

One concern I have about this is that I'm fairly certain this will complicate cancellation quite a bit. What I do now is a bit indirect, but everything is queued immediately in the appropriate place. Cancellation is then just a matter of revoking the slot in the queue. If I applied your suggestion, I would essentially be queuing the changes to the queues. I would still need the separate queues to handle the correct ordering of reads and takes, but I can't convince myself I couldn't get into a situation where it's possible to try to revoke a pending item when it hasn't been put in the appropriate queue yet. I would have to track this somehow.

I've also tried to keep allocations to a minimum. With this approach, takes/reads are only a single allocation per operation (aside from the usual PS overhead), and puts are two.

Correction, for takes/reads there is an allocation for the queue slot, and an allocation for the canceler thunk. For puts there is the cb/value tuple, the queue slot, and the canceler.

but I can't convince myself I couldn't get into a situation where it's possible to try to revoke a pending item when it hasn't been put in the appropriate queue yet

Thinking about this again, this is not a reasonable concern. If the queue is flushed, the clearly it will be in the appropriate place before yielding the canceler. Mutation hurts my brain.

I rescind that 😆. If you start a loop, and you queue something while in the loop, the canceler will return before the item is queued properly, so you'd have to track two places in the queues. In light of this, I think I'd rather just deal with it the way I've already done it since it requires no bookkeeping, at the expensive of the guards I've inserted.

jdegoes · 2017-07-06T16:23:23Z

src/Control/Monad/Eff/AVar.purs

+-- | Reads the AVar value. Unlike `takeVar`, this will not leave the AVar empty.
+-- | If the AVar is empty, this will queue until it is filled. Multiple reads
+-- | will resolve at the same time, as soon as possible.
+readVar ∷ ∀ eff a. AVar a → AVarCallback eff a → AVarEff eff (AVarEff eff Unit)


I like these guarantees. 👍

garyb · 2017-07-17T19:06:35Z

src/Control/Monad/Eff/AVar.js

+  this.takes    = new MutableQueue();
+  this.reads    = new MutableQueue();
+  this.puts     = new MutableQueue();
+}


Any reason not to make this accept/set value in the constructor?

Because you don't always want a value. There's no real difference. In the current case, if I want a value, I just mutate it. In the other case I'd have to provide EMPTY.

Yeah, that's all I was suggesting, that the other case became new AVar(EMPTY), but I don't mind really.

garyb · 2017-07-17T19:11:42Z

src/Control/Monad/Eff/AVar.js

+      return just(value);
+    }
+  };
+};


Do we not need to check error in this case? I see it's handled in drainVar too, but just wanted to double check, as it's checked in the non-try takeVar.

No, we don't. When an AVar is killed, it's value is set to EMPTY.

Ok, got it.

garyb · 2017-07-17T19:25:05Z

src/Control/Monad/Eff/AVar.purs

+foreign import _tryPutVar ∷ ∀ eff a. Fn.Fn4 (Error → Either Error a) (a → Either Error a) (AVar a) a (AVarEff eff Boolean)
+foreign import _takeVar ∷ ∀ eff a. Fn.Fn4 (Error → Either Error a) (a → Either Error a) (AVar a) (AVarCallback eff a) (AVarEff eff (AVarEff eff Unit))
+foreign import _tryTakeVar ∷ ∀ eff a. Fn.Fn5 (Error → Either Error a) (a → Either Error a) (Maybe a) (a → Maybe a) (AVar a) (AVarEff eff (Maybe a))
+foreign import _readVar ∷ ∀ eff a. Fn.Fn4 (Error → Either Error a) (a → Either Error a) (AVar a) (AVarCallback eff a) (AVarEff eff (AVarEff eff Unit))


We typically write Left / Right accepting arguments for the FFI as forall a b. a -> Either a b, etc. elsewhere as it means the implementation is obvious/only possible one way via parametricity.

natefaubion · 2017-07-18T03:16:57Z

Changed the signature to new AVar(initialValue)
Removed special error handling. Now everything just runs through drainVar.

garyb · 2017-07-18T11:11:36Z

🎉

natefaubion added 8 commits July 3, 2017 22:50

Initial implementation

44d6d98

Fix avar draining, add tests

213b668

Remove unnecessary boxing

a147466

Cancellation

39b21f6

Cleanup

828ed99

Comments

65f64f0

Minor cleanup

b219b8b

Add travis config

8128435

jdegoes reviewed Jul 6, 2017

View reviewed changes

Simplify error dispatch

dd1835b

garyb reviewed Jul 17, 2017

View reviewed changes

natefaubion added 2 commits July 17, 2017 20:09

Use parametric signatures for Either constructors

3c9a947

Change AVar signature to take value. Remove special error checking

62e39b8

natefaubion changed the title ~~WIP: Initial implementation~~ Initial implementation Jul 18, 2017

natefaubion merged commit e734e17 into purescript-contrib:master Jul 18, 2017

Initial implementation #1

Initial implementation #1

Uh oh!

Conversation

natefaubion commented Jul 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natefaubion commented Jul 18, 2017

Uh oh!

garyb commented Jul 18, 2017

Uh oh!

Uh oh!