AtomicWaker: take and wake #4151
Conversation
bugadani
force-pushed
the
take-and-wake
branch
2 times, most recently
from
02b503e to
d6e19ce
Compare
| @@ -156,8 +156,7 @@ fn waking_after_completion_does_not_poll() { | |||
| let (executor, trace) = setup(); | |||
| executor.spawner().spawn(task1(trace.clone(), waker)).unwrap(); | |||
|
|
|||
| unsafe { executor.poll() }; | |||
There was a problem hiding this comment.
I'm not sure what I was thinking here, the task is single shot and exits by the time this poll returns.
|
Hmm does that trusted label mean I can ask for...? bender run |
|
|
|
bender run |
|
I'm wondering, this alone doesn't fix the whole problem right? we also have to ensure all drivers deregister all their wakers on drop, otherwise they'll stay there forever keeping a reference to the task. And the extra code to do that would be "waste" when running with embassy-executor. |
|
No, not the whole problem, but with the follow-up PR the bits are there for HAL authors to do it if they want to, without their own AtomicWaker. |
|
Is this something you're considering doing in esp-hal? How're you planning of dealing with "the extra code to do that would be "waste" when running with embassy-executor"? just accept the bloat? |
We don't have targets with 32k flash, that's how 😅 This isn't exactly about what we consider doing, it's more about not limiting the HAL to statically allocated tasks. |
Once this (and the follow up PR lands), I want to make PRs to |
|
So, this PR should either be neutral, or size/perf-positive because there's no need to write the Waker back into the Cell. However, the next PR will not be - |
While in embassy tasks are static, in other environments it's usually not the norm. There, keeping the Waker may force the task that registered it to remain allocated, potentially indefinitely. Given that AtomicWaker is used in peripheral drivers that aren't tied to a particular async framework, letting go of the Waker once it's not needed is probably better than keeping it around.
I expect this change to surface some issues around Futures that only register the waker once and expect it to stick around.