Replace trait Wakeup with NotifyHandle?

[alexcrichton]

The recent addition of the Sleep trait also added an associated Wakeup trait for the associated type and its method, Sleep::wakeup, to return. This trait is, however, very similar to the Notify trait in spirit (modulo the workings with ids). In the spirit of reducing the number of traits we have, could the usage of Wakeup be replaced with NotifyHandle?

This'd have the added benefit of making Sleep more ergonomically object safe and I don't think we'd lose any functionality that Wakeup offers today. It should buy implementors the flexibility to do what they'd like as well as the users in the futures crate to use it in a generic fashion.

[carllerche]

I opted to introduce a new trait instead of using NotifyHandle because:

• I couldn't come up with a way to avoid double dynamic dispatch. Introducing Wakeup removed a dyn dispatch.
• 99% of all users of futures shouldn't need to know about Sleep and Wakeup.

Given these two points, it seemed to me that it was better to introduce a new trait instead of using NotifyHandle.

[alexcrichton]

Can you clarify what the double dispatch problem is? (it's not immediately clear to me where the second one is). Also, was it measured to be a performance hit or a source of slowness?

[carllerche]

The double dyn dispatch point isn't so much a point about performance, but code smell involved with using NotifyHandle. You could obviously leverage any number of other existing traits (Notify, NotifyHandle) to achieve a similar result if you don't care about passing in junk arguments.

If you are concerned about trait proliferation, it may make sense to look at Wakeup with all the existing traits and figure out which are needed vs. not.

For example, does Notify need the id argument, clone_id and drop_id or can that functionality be deferred to UnsafeNotify? In this case, Notify would become Wakeup.

[alexcrichton]

Ok, can you still clarify though that the double dispatch is? I know that NotifyHandle's notification is dynamically dispatched, but I wasn't sure where the other dynamic dispatch was?

I'm personally worried about too many traits, yeah, but we can always attempt to curb them with more documentation and examples, so it's not a strong point of objection by any means. I also agree yeah that NotifyHandle isn't a perfect fit here because of the id arguments (the Sleep trait would just document that the values don't matter too much).

I originally thought this was going to be required for #690 for eliminating a type parameter but it ended up not being necessary (hence the PR). I then attempted to tackle a separate issue (#697) in testing out a solution but that also required this issue.

Attempting to solve #697 in the naive fashion (calling executor::spawn on the return value) then requires calling poll_future_notify which requires a T: Clone + Into<NotifyHandle>. That can be done with an extra Arc allocation but most sleepers will probably already have the ability to do so via a NotifyHandle.

[carllerche]

A scheduler node implements Notify so that it can provide a NotifyHandle to its futures. The first dynamic dispatch goes via NotifyHandle::notify and reaches here.

Today, since the scheduler is generic over W: Wakeup, there is no further dynamic dispatch. If this W: Wakeup is replaced a NotifyHandle, this would add the second dynamic dispatch.

Again though, replacing Wakeup with the current Notify trait is equivalent, but without the extra dynamic dispatch. The real question is how good of a fit is that trait due to its function signatures (the same problem as using NotifyHandle).

[carllerche]

What are the actionable items remaining for this issue?

[aturon]

cc @cramertj

@alexcrichton, at some point you were talking about an approach that can avoid allocation and does not need ids. Did that pan out?

[alexcrichton]

@aturon oh that was part of a grander scheme for reworking the Notify trait, but I'd assumed it was tabled for it was a breaking change (this was awhile ago). I never circled back to it again, but not that we're considering an 0.2 release I think we'd want to consider doing that yeah, removing the ids from Notify and then the Wakeup trait could at least be replaced with Notify and we could maybe even use NotifyHandle in a few places.

[aturon]

Closing as no longer relevant.