Iteration policy API for eager polling loops #1961
Closed
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
mzabaluev
force-pushed
the
loop-guards
branch
from
e8e2fc4
to
d0b5910
Compare
mzabaluev
force-pushed
the
loop-guards
branch
from
d0b5910
to
5fa2ae7
Compare
|
This is a simple all-dynamic solution that augments futures and streams which may be affected by saturation of their poll function with a counter-based relief valve. A more elaborate, generic extension is also possible that will allow the users to opt out of the iteration counter statically, which will be backward compatible with these additions (except the |
|
I'm not sure about landing the |
mzabaluev
changed the title
Starting work on adding busy loop guards to stream combinators.
An unlabeled break expression inside the loop body given to poll_loop! breaks out of the loop in the expansion with the effect of yielding to the task. Make the users conscious of this by requiring the break expression to carry a token value of type $crate::task::ToYield.
Import the poll_loop! macro from futures-core. Define a constant with some uneducated guess value on a good default for yielding after this number of iterations, which should keep the overhead low in good cases, but not block the outer poll for too many iterations.
Not normally expected to occur, but a busy stream of empty streams can theoretically saturate this loop.
These can busy loop if the filter future consistenly filters out items.
Use NonNullU32 for the limits on the number of iterations to give the optimizer information that the first iteration will always be taken, allowing to unroll it or move the loop exit check to the end. To make poll_loop! macro work with a NonNullU32 argument, apply an Into<u32> conversion to it. The iteration range is now strictly in u32, while previously the integer type was inferred from the macro's argument.
Add combinator method .yield_after_every(n) to stream and future combinator types that have been provided with iteration guards. This gives the user means to fine tune the yielding behavior.
Future-proofing the poll_loop! macro by defining a Policy trait that is provided by poll guard implementations: Limit with the maximum count, and Unlimited that does not do any checks.
Perform the yield check at the end of an iteration. Document the check-skipping effect of `continue` and the new semantics of `break`: this can now be used to return arbitrary values from the poll_loop! expression. This is less semantically weird than `break ToYield`, and `continue` gets back its usual meaning of continuing iteration unconditionally.
iteration::Policy gets methods to observe how the loop has ended. LoopGuard, a RAII helper, is used by poll_loop! to catch early returns. The uses of poll_loop! in futures-util needed to be changed to split borrows out of Pin<&mut Self>, allowing the guard to borrow the iteration checker while other state objects are used and modified by the loop body. Notably, this makes the code of the loops more readable.
Add optional customization of a possibly nested path to the iteration policy checker field.
The default limit is nearly arbitrary, aiming to keep the yielding overhead in low single digit percentages while not starving other pollables for too many iterations. Making it a power of two does not make much difference, except that it may produce more efficient allocation patterns in hypothetical cases e.g. when a future accumulates a collection from polled items, then yields, which causes draining of the collection by some other pollable.
It's not a good idea to let impls run arbitrary code in the RAII destructor which may be invoked in an unwind. Also, separation between yield_check and when_yielded is not really necessary. Erase when_* methods on Policy and allow the yield_check method to modify the receiver so that adaptive and instrumentation impls are still supported. With this, LoopGuard is not needed in poll_loop! and is gone.
Renamed: Policy -> LoopPolicy, for more instant legibility in docs Policy::begin -> LoopPolicy::enter, clearer meaning
mzabaluev
force-pushed
the
loop-guards
branch
from
8f1256c
to
db4b867
Compare
Make the meta parameter names describe what each doc string refers to.
taiki-e
reviewed
Feb 13, 2021
There was a problem hiding this comment.
Sorry for the late review.
I would not prefer to provide abstractions or APIs like this on futures side for now. I also would not prefer to use the way of increasing the size of the combinators like this approach.
I'm not clear what the medium-term solution to this problem is (In the short-term, we will need to use an approach like #2333), but I think it will be resolved by std in the long-term.
So I'm going to close this PR. Thanks anyway @mzabaluev!
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.
Work to fix #1957 and provide a new macro for responsible polling.