Reimplement scheduler affinity #527
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
facebook-github-bot
added
the
CLA Signed
|
This depends on #526. |
ispeters
force-pushed
the
reimplement_scheduler_affinity
branch
2 times, most recently
from
23d1445 to
7bd7d60
Compare
|
To make this really work well, I'll need to follow up with customizations of |
ispeters
commented
May 24, 2023
ispeters
commented
May 24, 2023
ispeters
force-pushed
the
reimplement_scheduler_affinity
branch
from
7bd7d60 to
25aea72
Compare
ispeters
commented
May 31, 2023
Comment on lines
+321
to
+322
| // TODO: verify that `at_coroutine_exit()` can't be used to break scheduler | ||
| // affinity by running an async task that reschedules |
There was a problem hiding this comment.
at_coroutine_exit can already break Scheduler affinity so this is a TODO, which I've captured in #535.
ispeters
commented
May 31, 2023
This diff changes how `unifex::task<>` implements *Scheduler* affinity to work
more like `folly::coro::Task<>`. What Folly calls `co_viaIfAsync`, this diff
calls `with_scheduler_affinity`, which is a new CPO.
`with_scheduler_affinity` is a *Sender* algorithm that maps the given *Sender*
to another *Sender* that must meet new postconditions if the *Receiver* that
it's connected to meets certain preconditions. The new preconditions are:
- the eventual *Receiver* must provide a "current *Scheduler*";
- the result of `with_scheduler_affinity` must be started on the *Receiver's*
current *Scheduler*; and
- stop requests delivered to the new *Sender* must be delivered on the
*Receiver's* current *Scheduler*.
The *Sender* returned from `with_scheduler_affinity` must complete on its
*Receiver's* current *Scheduler* (i.e. must complete where it was started) so
long as all the above preconditions are met.
Any *Sender* type, `S`, that has `sender_traits<S>::is_always_scheduler_affine`
set to `true` will be returned unmodified from `with_scheduler_affinity`. For
*Senders* that are not always *Scheduler*-affine and that do not customize
`with_scheduler_affinity`, the default implementation is:
```
template <typename Sender, typename Scheduler>
auto with_scheduler_affinity(Sender&& s, Scheduler&& sched) {
return finally(
std::forward<Sender>(s),
unstoppable(schedule(std::forward<Scheduler>(sched))));
}
```
ispeters
commented
May 31, 2023
include/unifex/task.hpp
Outdated
| * nested stop source on the correct scheduler; and | ||
| * - ensure that, if the async stop request is ever started, we wait for | ||
| * *both* the async stop request *and* the nested operation to complete | ||
| * before continuating our continuation. |
There was a problem hiding this comment.
lol, "continuating"
ispeters
commented
May 31, 2023
ispeters
commented
May 31, 2023
This diff address the review comments I left myself, other than the ones I punted to issues. I also realized I could use a `manual_lifetime<>` instead of a `std::optional<>` for the `_sr_thunk_task`'s stop callback, which saves a little overhead.
ispeters
force-pushed
the
reimplement_scheduler_affinity
branch
from
25aea72 to
e002af2
Compare
janondrusek
reviewed
May 31, 2023
|
|
||
| template <typename Sender, typename Scheduler> | ||
| using wsa_sender_wrapper = | ||
| typename _wsa_sender_wrapper<Sender, Scheduler>::type; |
There was a problem hiding this comment.
nit: just _wsa_sender?
There was a problem hiding this comment.
Maybe next time I'm in here; I'd like to get this merged so we can sync it internally.
janondrusek
approved these changes
May 31, 2023
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 diff changes how
unifex::task<>implements Scheduler affinity to work more likefolly::coro::Task<>. What Folly callsco_viaIfAsync, this diff callswith_scheduler_affinity, which is a new CPO.with_scheduler_affinityis a Sender algorithm that maps the given Sender to another Sender that must meet new postconditions if the Receiver that it's connected to meets certain preconditions. The new preconditions are:with_scheduler_affinitymust be started on the Receiver's current Scheduler; andThe Sender returned from
with_scheduler_affinitymust complete on its Receiver's current Scheduler (i.e. must complete where it was started) so long as all the above preconditions are met.with_scheduler_affinityhas two default implementations for a Sender typeS:sender_traits<S>::is_always_scheduler_affineistrue, the default is the identity;sender_traits<S>::is_always_scheduler_affineisfalse, the default is, essentially, atyped_viaback to the correct scheduler.Any Sender can customize
with_scheduler_affinity, regardless of itsis_always_scheduler_affineproperty.