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

Add createConnectedBufferedChannels with simple delay model #1141

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

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented Oct 16, 2019

Crude approximation of GSV style.

-- | Create a pair of channels that are connected via buffers with delays.
--
-- This is a crude approximation of asynchronous network connections where
-- there is delay across the connection, and a limit on in-flight data.
--
-- The buffer size is bounded. It /blocks/ when 'send' would exceed the maximum
-- buffer size. The size per channel element is provided by a function, so this
-- can be size in bytes or a fixed size (e.g. 1 per element), so the max size
-- should be interpreted accordingly.
--
-- The delays are modeled in a simplistic "GSV" style:
--
-- * The G is the minimum latency for a 0 sized message.
-- * The S is the time per message size unit.
-- * The V is the variance in latency, which is assumed to be uniform in the
--   range @0..v@.
--
-- The sender is delayed by S. The receiver is delayed until the arrival time
-- which is G + S + V.
--
-- Note that this implementation does not handle the delays correctly if there
-- are multiple writers or multiple readers.
--
-- This is primarily useful for testing protocols.
--
createConnectedDelayChannels
  :: forall m prng a.
     (MonadSTM m, MonadTime m, MonadTimer m, RandomGen prng)
  => (a -> Int)   -- ^ Data size measure
  -> Int          -- ^ Max size of data in-flight
  -> (DiffTime, DiffTime, DiffTime) -- ^ GSV
  -> prng         -- ^ PRNG for sampling V
  -> m (Channel m a, Channel m a)

Test included to check that the delays we get are what we expect.

@dcoutts dcoutts requested review from coot and nfrisby October 16, 2019 11:04
@coot
Copy link
Contributor

coot commented Oct 16, 2019

-- Note that this implementation does not handle the delays correctly if there
-- are multiple writers or multiple readers.

The consensus tests are using a single channel per mini-protocol, so it should work.

-> g
-> [b]
-> [((VTime, b), (VTime, b))]
expectedDelayChannelTimes maxsize (g, s, v) prng0 xs0 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually surprising to me that one can dequeue only when the channel is full to correctly model channels with delay. I think that's possible because the time (the now argument of go) is decoupled and moves on as we dequeue.

Maybe you have a simple model how this works, it would be good to put it in haddock.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My "ah ha" moment along these lines was realizing that the scheduled "arrival times" of the elements in the buffer might not be monotonic -- so they're actually the "earliest possible arrival time" per message, not necessarily the actual arrival time. Hence arrive = max maxarrive ((g + vsample) `addTime` depart).

That contention/overlap between the queue's semantics and the scheduled arrival times' values was a bit surprising. Would it be worthwhile to maintain the max while generating the arrival times? Or perhaps just a comment/renaming some variables would suffice. (Possibly include whichever in the implementation too, so that debugging the queue's contents might be less confusing?)

More directly to Marcin's comment: to is called once per element, as expected. The queue maintained here is not analogous to the original queue of elements. A renaming might help: perhaps now -> senderTime and maxarrive to receiverTime? Or senderClock and receiverClock, etc.

Comment on lines -53 to +54
time
time,
random
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve alphabetic order?

Comment on lines 93 to +94
, time
, random
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve alphabetic order?

Comment on lines +115 to +117
----------------
-- Queue
--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is simple and good, but would reusing Data.Sequence be even simpler (e.g. less maintenance burden)?

]


prop_createConnectedDelayChannels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worthwhile to also test the other half of the Channel?

-> g
-> [b]
-> [((VTime, b), (VTime, b))]
expectedDelayChannelTimes maxsize (g, s, v) prng0 xs0 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My "ah ha" moment along these lines was realizing that the scheduled "arrival times" of the elements in the buffer might not be monotonic -- so they're actually the "earliest possible arrival time" per message, not necessarily the actual arrival time. Hence arrive = max maxarrive ((g + vsample) `addTime` depart).

That contention/overlap between the queue's semantics and the scheduled arrival times' values was a bit surprising. Would it be worthwhile to maintain the max while generating the arrival times? Or perhaps just a comment/renaming some variables would suffice. (Possibly include whichever in the implementation too, so that debugging the queue's contents might be less confusing?)

More directly to Marcin's comment: to is called once per element, as expected. The queue maintained here is not analogous to the original queue of elements. A renaming might help: perhaps now -> senderTime and maxarrive to receiverTime? Or senderClock and receiverClock, etc.

Comment on lines +180 to +184
-- * The V is the variance in latency, which is assumed to be uniform in the
-- range @0..v@.
--
-- The sender is delayed by S. The receiver is delayed until the arrival time
-- which is G + S + V.
Copy link
Contributor

@nfrisby nfrisby Oct 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain, but those three sentences together feel somewhat incorrect. Something like "the receiver is delayed until either G + S + V, or by something more than that because the channel had to preserve order instead of respecting that particular scheduled arrival time."

This is the same thing I mentioned in the test code.

@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 27, 2020

@nfrisby I presume we don't need this now right? Or would it still be useful to tidy this up?

@nfrisby
Copy link
Contributor

nfrisby commented Mar 1, 2020

tl; dr - I'm not currently planning on relying on this.

I used a different implementation for my PR that was doing similar things. But it's been so long that I can't really explain the difference -- I think we wrote them in parallel, if I recall correctly. They might have slightly different invariants too, with a particular focus on my intention to make sure the accumulated latencies didn't breach over into the next slot, since that is in some ways a "network partition" at that point. My test Propertys still aren't ready for those :(

@JaredCorduan JaredCorduan requested a review from bolt12 as a code owner April 24, 2023 22:50
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.

3 participants