substream-set: Poll substreams fairly with poll indexes #228
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
|
I'll do more testing with this PR since one test is failing because we expect fewer polls to happen |
and use IndexMap Signed-off-by: Alexandru Vasile <[email protected]>
|
In this revision, I've replaced the One test was previously failing due to the poll_index not incrementing properly |
| return None; | ||
| }; | ||
|
|
||
| self.waker.take().map(|waker| waker.wake()); |
There was a problem hiding this comment.
I don't think we need to wake the task when removing a substream. I.e., all the remaining futures in the map were already registered in the task context.
|
|
||
| for _ in 0..len { | ||
| let index = inner.poll_index % len; | ||
| inner.poll_index = (inner.poll_index + 1) % len; |
There was a problem hiding this comment.
Why not just toss the starting index randomly when poll_next is called? This way we won't need to keep the last starting index, and the polling will be even more random than when just offsetting by 1. I.e., when we offset the index by one there are still more chances to poll substreams from the first half of the list until the starting index offsets significantly.
Also, I would even keep a HashMap for additional shuffling instead of using IndexMap that keeps the insertion order. (We will need to traverse the HashMap twice to implement iteration starting from some offset. This should not be a problem, but double check the order of the HashMap iteration is the same if the map hasn't changed between the iterations, please.)
|
Planning to remove this entirely in the future, this is no longer needed |
This PR refactors the substream-set (used by request-response protocols):