Simplify maybeOpenRxWindow and improve docs
#7
Conversation
|
I'll see if I can fix the tests (see also #6 ). |
|
Fixed the tests (so this closes #6); I've changed the test approach a bit; see code. I will also run a few benchmarks. |
|
Ok, discussed this with @FinleyMcIlwaine , we think this PR is good to go. (I fixed two minor tihngs in the comments.) |
Network/Control/Flow.hs
Outdated
| @@ -81,25 +79,55 @@ txWindowSize TxFlow{..} = txfLimit - txfSent | |||
|
|
|||
| -- | Flow for receiving. | |||
| -- | |||
| -- The goal of 'RxFlow' is to ensure that our network peer does not send us data | |||
| -- faster than we can consume it. We therefore impose a buffer size (also known | |||
| -- as a _window_) of unconsumed bytes that we are willing to receive from the | |||
There was a problem hiding this comment.
In my understanding, window is the buffer size which is not filled by peer, not the total size of the buffer.
So, this explanation is a bit confusing.
There was a problem hiding this comment.
Have rewritten this as
The goal of 'RxFlow' is to ensure that our network peer does not send us data faster than we can consume it. We therefore impose a maximum number of unconsumed bytes that we are willing to receive from the peer, which we refer to as the buffer size:
Network/Control/Flow.hs
Outdated
| rxfConsumed' = rxfConsumed + consumed | ||
|
|
||
| -- Minimum window update size | ||
| threshold = rxfBufSize `div` 2 |
There was a problem hiding this comment.
Why don't you use unsafeShiftR?
There was a problem hiding this comment.
Changed this to be back at its original definition:
threshold = rxfBufSize `unsafeShiftR` 1
|
@kazu-yamamoto Thanks for your review; I've addressed your comments. Let me know if you'd like any other changes! |
|
Merged and released. |
## 0.1.4 * Using Integer instead of Int in LRUCache. ## 0.1.3 * Simplify `maybeOpenRxWindow` and improve docs [#7](kazu-yamamoto/network-control#7) ## 0.1.2 * introducing a minimum size for window update [#5](kazu-yamamoto/network-control#5) ## 0.1.1 * Change defaultMaxData [#4](kazu-yamamoto/network-control#4)
@kazu-yamamoto This PR introduces the simplified algorithm @FinleyMcIlwaine alluded to in #5 (comment) . I don't think this PR is critical; if you prefer the algorithm as-is, feel free to simply close this PR; but since you asked, here it is. The main idea is simply that we send a window update as soon as we have consumed more than half the window; after all, the goal of rate limiting is to ensure the peer cannot send us data faster than we can consume it.
I have also tried to explain in the documentation of
RxFlow(my understanding of) what is happening. The diagram is very similar to what you had originally, but not quite the same; in particular,rxfLimitsits within the window. See text for details.I haven't had a chance yet to discuss this with @FinleyMcIlwaine (he is in a different time zone to me), so if you do agree that this is a good please hold off on merging for just a bit, until he and I can discuss also.