Anti-pipelining (TODO better name) (AKA receiving despite having agency)#92
Anti-pipelining (TODO better name) (AKA receiving despite having agency)#92nfrisby wants to merge 2 commits into
Conversation
…nder This way, the driver state can be just a number instead of a queue.
|
I don't have it all wired up, so I haven't even seen this running in a prototype yet. But, as demonstration, here's my current best guess at the definition of the leiosNotifyServerPeerAntiPipelined ::
forall m point announcement vote.
MonadThrow m =>
m Bool ->
-- ^ increments the number of outstanding requests
m (Message (LeiosNotify point announcement vote) StBusy StIdle) ->
-- ^ blocks until the next reply (announcement\/offer\/vote) is ready
PeerAntiPipelined (LeiosNotify point announcement vote) AsServer StIdle m ()
leiosNotifyServerPeerAntiPipelined incr next =
PeerAntiPipelined responder (go Zero)
where
responder :: Sender (LeiosNotify point announcement vote) AsServer StBusy StIdle m
responder = SenderEffect $ next <&> \msg -> SenderYield ReflServerAgency msg SenderDone
go :: forall n.
Nat n ->
Peer (LeiosNotify point announcement vote) AsServer (AntiPipelined StBusy StIdle n) StIdle m ()
go n =
Await ReflClientAgency $ \case
MsgDone -> drain n
MsgLeiosNotificationRequestNext ->
Effect $ do
incr >>= \b -> when b $ throwIO MkExnLeiosNotifyExcessiveRequests
pure $ YieldAntiPipelined ReflServerAgency (go (Succ n))
-- on termination, flush the sends we've handed off, then Done.
drain :: forall n.
Nat n ->
Peer (LeiosNotify point announcement vote) AsServer (AntiPipelined StBusy StIdle n) StDone m ()
drain = \case
Zero -> Done ReflNobodyAgency ()
Succ j -> AntiCollect (drain j) Nothing
data ExnLeiosNotifyExcessiveRequests = MkExnLeiosNotifyExcessiveRequests
deriving Show
instance Exception ExnLeiosNotifyExcessiveRequests |
|
Oh, another thing: suppose the client has pipelined 300 requests and then also pipelines This LeiosNotify server as-is cannot terminate until it generates 300 responses, which might take a while (if the protocol happens to be idle). But: @coot has long been saying we need to add a "heart beat" message: an empty response that the server sends on some period of inactivity and that the client accepts if it's not sent too often (with a slightly asymmetric send rate and allowed receive rate for clockskew/transmission delay/buffering/etc). If we were to add that, then the (edit: this "prompt-drain" sounds quite useful, and applicable to other client- |
|
Ah, another possible reason to consider
Sounds nice and simple, but the query can be non-trivial to define, might be expensive, and require maintaining extra state. Even worse: it might require maintaining extra state per peer---recall again that one LeiosNotify server runs per downstream peer and there can be hundreds of those. So I'm trying to avoid this. It's worth emphasizing that all of our existing mini protocol servers do this. However, my intuition (which I admittedly haven't already unpacked) is that LeiosNotify is less compatible with it than the Praos mini protocols are: ChainSync and BlockFetch both have very very simple server logic (the thing we run hundreds of). I think that was also true for old TxSubmission; I'm not sure about new TxSubmission (b/c it has centralized decision logic now). |
coot
left a comment
There was a problem hiding this comment.
Have you tried to extend proofs so Pipelined and AntiPiplined peers can run against each other? That's the fun/challenging part of typed-protocols 😁
| -> Peer ps pr (AntiPipelined st st' (S n)) st' m a | ||
| -- ^ continuation, before or after sending |
There was a problem hiding this comment.
Why not require Sender here, as the Receiver is passed in YeildPipelined?
That's probably why you said in the PR description that all Senders in your use cases are the same.
There was a problem hiding this comment.
Correct: yeah, excluding the Sender here means we don't need the driver to maintain a queue.
|
The forgetAnitPipelined :: [Bool]
-> PeerAntiPipelined ps pr st m a
-> Peer ps pr m awhich inlines the |
| NonPipelined :: IsPipelined | ||
| NonPipelined :: IsPipelined ps | ||
|
|
||
| -- | Pipelined peer for a /server/ that only ever uses one |
There was a problem hiding this comment.
You posted a top-level question here about "proofs". #92 (review)
- I'm posting it here (arbitrary) so we can have a threaded conversation about it.
- Do you mean the Agda? I seem to recall some (previous version?) Haskell type classes whose methods were "proofs", and i was expecting GHC to force me to incorporate
AntiPipelinedinto them at some point, but it never did. - I would love to be paid to work with Agda, but I never have been; so I'm probably not the most efficient person to tackle these proofs. (... take it with a grain of salt, but my intuition is that it should be simple/very similar to the existing
Pipelinedproof).
There was a problem hiding this comment.
No, in our most loved theorem prover - Haskell 😉. If we implement forgetAntiPielined then we can have a connectAntiPipelined similar to connectPipeliend
There was a problem hiding this comment.
Do you mean the Agda? I seem to recall some type classes whose methods were "proofs", and i was expecting GHC to force me to incorporate AntiPipelined into them at some point, but it never did.
That's because you extended the type and guarded it at the type level with AniPipelined constructor.
There was a problem hiding this comment.
🤔 if our proofs where polymorphic in IsPipelined argument, then it would force you for write a proof, but connect requires NonPipelined peers, and connectPipelined requires Pipelined ones.
|
What do you think about these names:
Please also add pattern synonyms in |
I'm currently upgrading the LeiosNotify mini protocol in the
leios-prototypebranch ofouroboros-consensusbe closer to what I've been intended with the design so far.This PR was the result of me suddenly remembering yesterday that
typed-protocolsdoesn't already allow what I had in mind (but I was confident it was feasible).The key idea: my intention has been for the LeiosNotify server to maintain a buffer of messages it would like to send. The capacity of that buffer should be NumRequestsThatHaveArrived - NumRepliesAlreadySent. If its capacity ever grows beyond some limit, we disconnect: they sent too many requests without waiting for responses.
Implementing that in
typed-protocolsrequires letting thePeerthat has agency receive a message (which can only happen if the otherPeerisPipelined). In other words, the send can be non-blocking. This is opposite/dual of whatYieldPipelinedalready does, in which a receive is non-blocking. HenceYieldAntiPipelinedetc (but I'd be happy adopt suggestions for a better name).A few comments:
AntiPipelinedand the other is notPipelined. That seems mild enough, though, since that's a pretty severe mistake to have made.Peercould be bothPipelinedandAntiPipelinedat the same time. But I don't need that, so I didn't pursue it.YieldAntiPipelinedctor carry theSenderand the driver would maintain a queue ofSendersinstead of just the counter this PR uses. But I'm anticipating quite big LeiosNotify pipeline depths (and this is on the upstream side, so there can be hundreds of theseAntiPipelinedPeers running), so I instead required that everySenderis the same, so that the driver state is merely a counter instead of a queue. (The LeiosNotify client also shares that same large pipelining depth, and it's currently using a queue of(). That's not ideal and maybe worth an eventual PR, but it's at least only one per upstream peer (tens), rather than one per downstream peer (hundreds).)Lastly, an admission: this all turned out roughly how I thought it would. But then it occurred to me that perhaps there's a simpler approach to LeiosNotify. Since we're bounding how many outstanding requests there are, we could just use a normal
NonPipelinedPeerfor the server, and have its request handler read from a bounded queue that's not allowed to grow beyond that pipelining depth (and that queue basically exists outside of thePeerEDSL/driver).I can only see a couple benefits to
AntiPipelined.NonPipelinedPeerpulls from whenever it callsAwait. But suppose we had no responses to send for a while (ie the queue was empty). Then the only thing stopping the downstream peer from filling up that bearer's buffer is a low-level size limit on that buffer itself. Maybe that's tolerable?AntiPipelinedserverPeerautomatically has a queue capacity of 0 before any requests arrive is quite preferable---this is "precision" that it seems nice to be able to take for granted.