-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add provider-style API to Context
#135051
base: master
Are you sure you want to change the base?
Conversation
The job Click to see the possible cause of the failure (guessed by this bot)
|
@@ -303,6 +545,8 @@ impl fmt::Debug for Context<'_> { | |||
pub struct ContextBuilder<'a> { | |||
waker: &'a Waker, | |||
local_waker: &'a LocalWaker, | |||
provider: Option<&'a mut (dyn Provider + 'static)>, | |||
parent_provider: Option<&'a mut (dyn Provider + 'static)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it support more levels of inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is to traverse the chain of contexts/providers but now that you mention it I realized the code is not actually doing this.
I've got two concerns:
|
I wonder how important this is. In my experience,
Certainly if there are a lot of lookups for different interfaces. I think the best we could hope for is a single lookup being comparable to a thread local. In other words, ideally these two would have similar performance: let rs: &RuntimeStuff = &RUNTIME_STUFF_LOCAL.with(|k| Rc::clone(k)));
let rs: &RuntimeStuff = cx.request_ref::<RuntimeStuff>().unwrap(); Then at the very least a runtime could use the context to find its own stuff, even if that does little to help with the overall goal of runtime interop. But maybe even this much is not true or possible with a provider-style API. |
Thanks for the PR @jkarneges! This is the shape of an API I was hoping to see.
As a mitigation to the memory size concerns, it might be possible to embed the parent pointer inside the Provider object instead of storing two pointers in the Context. I can see how crafting an API around this is tricky, though.
I hear your concern and agree we should try to avoid this outcome. At the same time, I don't want to put too many barriers in front of experimentation.
I agree that we need to think about code size; I think for highly constrained environments we are going to have to rely on libraries having feature flags to remove runtime detection for reactors etc using this API. My guess is that those environments largely won't need the features (or ergonomics) enabled by this API, though I could be wrong about that.
You may be right, but to challenge a bit, this would be on every suspension when we are waiting on IO. I don't see how the cost of doing an indirect call and comparing some integers is comparable to the time spent waiting. Yes that's time you could be spending on other stuff, but it still seems small by comparison. Ultimately I think this is a question we could answer empirically by adding the API in nightly and allowing people to experiment. Perhaps we should put some parameters around it to make sure it doesn't stay forever without getting stabilized (e.g. as a strawman, revisit in 12 months). |
r? libs |
Perhaps the fields can be gated with a Cargo feature so they don't increase the size of |
@Dirbaio What about a default-enabled gate? My feeling is that we should lower the bar for experimentation as much as possible in the common case, where the extra bytes aren't a big deal. |
how'd that work? there's no equivalent for Also, there'd be no way to disable it on stable because build-std is unstable, unless we also cfg out the fields on the prebuilt std shipped with stable. |
Did this get an ACP? I think it needs one, or at least a reason why it doesn't. |
Ah, I thought ACPs were optional if a code author is willing to take the risk of their code getting rejected, which I'm fine with. |
☔ The latest upstream changes (presumably #135937) made this pull request unmergeable. Please resolve the merge conflicts. |
This change enables
Context
to carry arbitrary extension data via an API similar to the one used byerror_generic_member_access
. It is intended to supersedecontext_ext
. It reusesRequest
and other types directly fromcore::error
, but it could be good to move those things to a common area if both the error & context usages are stabilized.Basic usage:
Currently,
Context
only carries aWaker
, but there is interest in having it carry other kinds of data. Examples include LocalWaker, a reactor interface, and multiple arbitrary values by type. There is also a general practice in the ecosystem of sharing data between executors and futures via thread-locals or globals that would arguably be better shared viaContext
, if it were possible.This change addresses some issues in the earlier proposed
context_ext
:context_ext
was intended to be stabilized without knowing what the most desirable data structure/mechanism for managing multiple extensions might be, in order to enable the ecosystem figure it out. Instead, this provider-style API is intended to be the desirable mechanism from the get-go. It supports returning multiple types, and overriding is possible by forwarding calls.Inheritance
Combinators that manage their own wakers will want to ensure the provider from an earlier context is carried over into any new contexts. For this, the
ContextBuilder::from()
method fromcontext_ext
can be used.Overriding extensions
Not only do we want a way for
Context
to carry multiple extensions, which the provider-style API solves, we also want a way for extensions to be added/overridden later on. For example, an executor may want to expose "reactor" & "spawner" interfaces from the top level, but then deeper in the call stack a "nursery" may want to override just the spawner (while allowing the reactor to still be accessible), and yet some other code path may want to introduce a "budgeter" for fair/weighted processing of some subset of work. These effects are achieved by having the context forward the provider calls to the parent provider after usingContextBuilder::from()
.Example overriding a "spawner":