Skip to content
This repository has been archived by the owner on Oct 17, 2022. It is now read-only.

feat: Avoid blocking tokio::select branches on a potent. full channel #724

Merged
merged 8 commits into from
Aug 18, 2022

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Aug 8, 2022

This is a re-do of #705.
Checkpointing some (incomplete) backpressure work on top of #691.
This PR is an incremental improvement.

  • This was previously reviewed as Avoid blocking tokio::select branches on a potent. full channel #705.
  • The problem that PR tried to solve was once the outgoing queue was full, you'd block the other branches of the tokio::select,
  • The issue with that PR was that once you fixed that, our waiting : Futures{Un,O}rdered would fill up indefinitely,
  • This new version of the PR creates BoundedFutures{Un,O}rdered waiting buffers, gives them (arguably) sensible defaults, and uses preconditions to gate select branches.

Todo

  • bound the waiting futures in the subscriber,
  • bound the waiting futures in the certificate_waiter,
  • bound the waiting futures in the header_waiter,

@huitseeker huitseeker requested a review from asonnino as a code owner August 8, 2022 16:28
@huitseeker huitseeker marked this pull request as draft August 8, 2022 16:28
@huitseeker huitseeker force-pushed the backpressure-2 branch 2 times, most recently from 991f17c to e08ff1f Compare August 17, 2022 03:13
@huitseeker huitseeker marked this pull request as ready for review August 17, 2022 03:13
@huitseeker huitseeker force-pushed the backpressure-2 branch 2 times, most recently from ab5bdfb to 90f7a37 Compare August 17, 2022 16:31
Copy link
Contributor

@akichidis akichidis left a comment

Choose a reason for hiding this comment

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

This is excellent work @huitseeker 💯 !! Left a couple of nit comments

executor/src/subscriber.rs Outdated Show resolved Hide resolved
primary/src/certificate_waiter.rs Outdated Show resolved Hide resolved
types/src/bounded_future_queue.rs Show resolved Hide resolved
pub fn try_next(&mut self) -> impl Future<Output = Result<Option<U>, V>> + '_ {
self.queue.try_next().inspect_ok(|val| {
if val.is_some() {
self.push_semaphore.add_permits(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to add a permit only in case of successful response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is a TryStreamExt::try_next, so the semantics of this returning an Option::None are the same as StreamExt::next, i.e. the stream is empty. In which case its available permits is already at capacity, and adding more permits would raise it above the capacity.

Copy link
Contributor Author

@huitseeker huitseeker Aug 18, 2022

Choose a reason for hiding this comment

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

In c535706 I've edited tests to reflect what happens with the capacity when you do try_next() past the end of the stream. I checked those tests do not pass if you remove the corresponding if val.is_some() guards.

@huitseeker huitseeker force-pushed the backpressure-2 branch 3 times, most recently from 93085a7 to 4eb7523 Compare August 18, 2022 21:48
@huitseeker huitseeker changed the title Avoid blocking tokio::select branches on a potent. full channel #705 feat: Avoid blocking tokio::select branches on a potent. full channel #705 Aug 18, 2022
@huitseeker huitseeker changed the title feat: Avoid blocking tokio::select branches on a potent. full channel #705 feat: Avoid blocking tokio::select branches on a potent. full channel Aug 18, 2022
@huitseeker huitseeker enabled auto-merge (squash) August 18, 2022 22:00
@huitseeker huitseeker merged commit 52dd0c4 into MystenLabs:main Aug 18, 2022
huitseeker added a commit to huitseeker/narwhal that referenced this pull request Aug 25, 2022
…MystenLabs#724)

* feat: don't block on sending on a full channel in primary

We block on sending to a channel that may be full.
This rather changes the behavior to poll the channel before blocking indefinitely on it.

Percolates backpressure one level up from the output channels of:
- header_waiter,
- certificate_waiter,

* feat: don't block on sending on a full channel in executor

* feat: add a bounded queue for FuturesUnordered

* feat: Bound pending elements in certificate waiter & header waiter

* feat: Bound pending elements in subscriber

* fix: edit tests to check `try_next()` relationship w/ capacity

* feat: add & update metrics for waiting lists in {certificate, header}waiter

* feat: add metric for waiting elements to subscriber
huitseeker added a commit that referenced this pull request Aug 25, 2022
…#724)

* feat: don't block on sending on a full channel in primary

We block on sending to a channel that may be full.
This rather changes the behavior to poll the channel before blocking indefinitely on it.

Percolates backpressure one level up from the output channels of:
- header_waiter,
- certificate_waiter,

* feat: don't block on sending on a full channel in executor

* feat: add a bounded queue for FuturesUnordered

* feat: Bound pending elements in certificate waiter & header waiter

* feat: Bound pending elements in subscriber

* fix: edit tests to check `try_next()` relationship w/ capacity

* feat: add & update metrics for waiting lists in {certificate, header}waiter

* feat: add metric for waiting elements to subscriber
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 30, 2022
…MystenLabs/narwhal#724)

* feat: don't block on sending on a full channel in primary

We block on sending to a channel that may be full.
This rather changes the behavior to poll the channel before blocking indefinitely on it.

Percolates backpressure one level up from the output channels of:
- header_waiter,
- certificate_waiter,

* feat: don't block on sending on a full channel in executor

* feat: add a bounded queue for FuturesUnordered

* feat: Bound pending elements in certificate waiter & header waiter

* feat: Bound pending elements in subscriber

* fix: edit tests to check `try_next()` relationship w/ capacity

* feat: add & update metrics for waiting lists in {certificate, header}waiter

* feat: add metric for waiting elements to subscriber
@huitseeker huitseeker deleted the backpressure-2 branch January 21, 2023 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants