coop: Undo budget decrement on Pending #2549
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch updates the coop logic so that the budget is only decremented if a future makes progress (that is, if it returns `Ready`). This is realized by restoring the budget to its former value after `poll_proceed` _unless_ the caller indicates that it made progress. The thinking here is that we always want tasks to make progress when we poll them. With the way things were, if a task polled 128 resources that could make no progress, and just returned `Pending`, then a 129th resource that _could_ make progress would not be polled. Worse yet, this could manifest as a deadlock, if the first 128 resources were all _waiting_ for the 129th resource, since it would _never_ be polled. The downside of this change is that `Pending` resources now do not take up any part of the budget, even though they _do_ take up time on the executor. If a task is particularly aggressive (or unoptimized), and polls a large number of resources that cannot make progress whenever it is polled, then coop will allow it to run potentially much longer before yielding than it could before. The impact of this should be relatively contained though, because tasks that behaved in this way in the past probably ignored `Pending` _anyway_, so whether a resource returned `Pending` due to coop or due to lack of progress may not make a difference to it.
Darksonn
added
A-tokio
|
CI failure looks to be #2499. |
carllerche
reviewed
May 20, 2020
carllerche
approved these changes
May 21, 2020
jensim
pushed a commit
to jensim/tokio
that referenced
this pull request
Jun 7, 2020
This patch updates the coop logic so that the budget is only decremented if a future makes progress (that is, if it returns `Ready`). This is realized by restoring the budget to its former value after `poll_proceed` _unless_ the caller indicates that it made progress. The thinking here is that we always want tasks to make progress when we poll them. With the way things were, if a task polled 128 resources that could make no progress, and just returned `Pending`, then a 129th resource that _could_ make progress would not be polled. Worse yet, this could manifest as a deadlock, if the first 128 resources were all _waiting_ for the 129th resource, since it would _never_ be polled. The downside of this change is that `Pending` resources now do not take up any part of the budget, even though they _do_ take up time on the executor. If a task is particularly aggressive (or unoptimized), and polls a large number of resources that cannot make progress whenever it is polled, then coop will allow it to run potentially much longer before yielding than it could before. The impact of this should be relatively contained though, because tasks that behaved in this way in the past probably ignored `Pending` _anyway_, so whether a resource returned `Pending` due to coop or due to lack of progress may not make a difference to it.
hawkw
added a commit
that referenced
this pull request
Jul 20, 2020
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621, #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567, #2520, #2521, #2572, #2493) - rt: allow calls to `block_on` inside calls to `block_in_place` that are themselves inside `block_on` (#2645) - net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597) - io: improve stack usage by allocating large buffers on directly on the heap (#2634) - io: fix unsound pin projection in `AsyncReadExt::read_buf` and `AsyncWriteExt::write_buf` (#2612) - io: fix unnecessary zeroing for `AsyncRead` implementors (#2525) - io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654) - coop: returning `Poll::Pending` no longer decrements the task budget (#2549) - io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods (#1915) - io: fix panic in `AsyncReadExt::read_line` (#2541) - task: add [`tracing`] instrumentation to spawned tasks (#2655) - sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors) (#2615) - net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604) - fs: add `OpenOptionsExt` for `OpenOptions` (#2515) - fs: add `DirBuilder` (#2524) [`tracing`]: https://crates.io/crates/tracing Signed-off-by: Eliza Weisman <[email protected]>
hawkw
added a commit
that referenced
this pull request
Jul 22, 2020
# 0.2.22 (July 2!, 2020) ### Fixes - docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621, #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567, #2520, #2521, #2493) - rt: allow calls to `block_on` inside calls to `block_in_place` that are themselves inside `block_on` (#2645) - net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597) - io: improve stack usage by allocating large buffers on directly on the heap (#2634) - io: fix unsound pin projection in `AsyncReadExt::read_buf` and `AsyncWriteExt::write_buf` (#2612) - io: fix unnecessary zeroing for `AsyncRead` implementors (#2525) - io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654) - io: fix panic in `AsyncReadExt::read_line` (#2541) ### Changes - coop: returning `Poll::Pending` no longer decrements the task budget (#2549) ### Added - io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods (#1915) - task: add [`tracing`] instrumentation to spawned tasks (#2655) - sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors) (#2615) - net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604) - fs: add `OpenOptionsExt` for `OpenOptions` (#2515) - fs: add `DirBuilder` (#2524) [`tracing`]: https://crates.io/crates/tracing Signed-off-by: Eliza Weisman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch updates the coop logic so that the budget is only decremented if a future makes progress (that is, if it returns
Ready). This is realized by restoring the budget to its former value afterpoll_proceedunless the caller indicates that it made progress.The thinking here is that we always want tasks to make progress when we poll them. With the way things were, if a task polled 128 resources that could make no progress, and just returned
Pending, then a 129th resource that could make progress would not be polled. Worse yet, this could manifest as a deadlock, if the first 128 resources were all waiting for the 129th resource, since it would never be polled.The downside of this change is that
Pendingresources now do not take up any part of the budget, even though they do take up time on the executor. If a task is particularly aggressive (or unoptimized), and polls a large number of resources that cannot make progress whenever it is polled, then coop will allow it to run potentially much longer before yielding than it could before. The impact of this should be relatively contained though, because tasks that behaved in this way in the past probably ignoredPendinganyway, so whether a resource returnedPendingdue to coop or due to lack of progress may not make a difference to it.