Waker::will_wake() gets mostly defeated by executor optimizations

[Matthias247]

The Futures Waker API provides a Waker::will_wake() method which tries to determine whether the 2 Wakers are equivalent. It intends to allow Futures so determine whether they have to swap a Waker or have to can keep an already stored one which is equivalent. That can avoid some churn on atomic refcounts on those Wakers.

The will_wake method is implemented by a PartialEq derive on RawWaker - which means will_wake() will return true only if the pointer as well as the vtable in RawWaker are equivalent.

However futures executors are moving more and more towards a design where they lazily create the "real" storeable Waker, and only provide a Waker reference to tasks they poll. E.g. futures-rs uses WakerRef tokio does the same. This avoids an atomic increment and decrement cycle for each task that an executor polls.

However this means the RawWaker that the executor passes through the Context parameter will now always be different to the one stored inside a Future and will_wake() will return false. Which causes the Future to update the Waker reference (2 atomic ops). If the executor polls a couple of sub-tasks which now all swap out their Wakers the original executor optimization could now even lead to de-optimization - since the result is more atomic ops in other places.

Using the will_wake() method makes most sense exactly inside the .poll() method of a Future in order to determine whether a stored Waker needs to get updated. This is now the exact same point where the WakerRef would return the false negative. Therefore using .will_wake() is not that helpful given the state of the executor ecosystem.

So far for the description of the issue. The next question is whether this can be improved or solved. I don't think changing PartialEq for RawWaker - e.g. to conly compare the data pointer - makes sense. It will likely only lead to false positives (waker.will_wake(other) == true). Which then leads to missing wakeups aka live-locks.

The original design of RawWaker plus Vtable contained a vtable entry for will_wake(). This would have definitely helped to solve the issue, since the executors would have been able to overwrite the check and to be able to associate Wakers and their WakerRefs. However this was removed for simplification purposes.

I think one possible outcome could be to keep this issue as a tracking issue for the deficit. And if there is ever a change to the vtable for other reasons to also add a will_wake method back. Up to then it's probably not worth an update.

[Matthias247]

cc @cramertj , @carllerche regarding executors

Given that maybe using WakerRef is not always a clear win. My understanding is that the main drawback will happen for tasks which poll() again even if the Future was not ready. Which happens mostly when using select! variants. Code without select should be good, because the task should only get woken for re-polling when progress can be made.

Maybe needs some investigations in real applications whether it's a net win or loss there.

[tmiasko]

Most executors I have seen so far tend to use a waker reference type merely to
avoid increasing the atomic reference count when creating the context. In such
a situation a simple solution is available: use exactly the same vtable for
both a normal waker and a waker reference type.

Assume that the waker reference type does not drop the waker (so that there is
no need for special no-op drop method in vtable). Then, neither drop nor
wake methods might be called on the waker reference type without cloning it
first, and it is fine to share the same vtable.

[Matthias247]

@tmiasko You are right, the vtable currently mostly seems to be different for drop(), and it could be worked around by using the same drop - and by forgetting the WakerRef instead of dropping it. So that seems to be a workaround for the time being.

It's obviously a bit hacky - since you get a memory leak if you forget the forget - but since WakerRefs and executors are very tightly coupled it might not be the biggest concern.