StreamMuxer methods should take &mut self

[Demi-Marie]

Currently, StreamMuxer is expected to be Sync, and all of its methods take &self. This forces implementations to do costly locking internally.

Since most uses of StreamMuxer use a single task for the entire connection, this overhead can be avoided in the common case. In the rare case where this does not hold, messages can be passed asynchronously.

[tomaka]

We discussed this a while ago and I agree with the idea, but this is a hypothetical change and given the complexity it's probably not going to happen any time soon.

[Demi-Marie]

What would need to be done to implement this?

[tomaka]

(assuming we don't want to simply wrap StreamMuxer around a Mutex, as that would kill the point)

Having &mut self means that can no longer build multiple structs that borrow the same StreamMuxer at the same time. In other words, SubstreamRef would have to disappear.

If we can no longer have an implementation of AsyncRead/AsyncWrite that corresponds to a single substream, the NodeHandler, InboundUpgrade, OutboundUpgrade, and ProtocolsHandler traits would have to be completely redesigned, and all the code that implements these traits rewritten to use a different programming paradigm.

Alternatively, we could keep an object that implements AsyncRead/AsyncWrite and does some sort of intermediate buffering of the messages before they are passed to the StreamMuxer. But to me that should be inside of the StreamMuxer itself.

[tomaka]

It's one of these issues where yes, performance is slightly degraded, but doing things in a true zero-cost way would considerably increase the complexity of the code.

[Demi-Marie]

What would that zero-cost way be? As is so often the case, callbacks are zero-cost, but they are not a fun programming paradigm.

One alternative is to drop the requirement that StreamMuxer be Sync. This would allow the use of cheap (!Sync) interior mutability, and allow AsyncRead and AsyncWrite to continue to be used. What do you think @tomaka?

[Demi-Marie]

Expecting that a StreamMuxer can be used from multiple threads is the source of the overhead. Allowing multiple references to a StreamMuxer is find, so long as those references are all used in only one thread.

[Demi-Marie]

It is also worth noting that, in light of io_uring and IOCP, AsyncRead and AsyncWrite are themselves not zero-cost.

[tomaka]

One alternative is to drop the requirement that StreamMuxer be Sync. This would allow the use of cheap (!Sync) interior mutability, and allow AsyncRead and AsyncWrite to continue to be used. What do you think @tomaka?

Unfortunately, since Futures jump between threads, they have to implement Send.

And if, within that Future, you hold a reference to the StreamMuxer (which would be the case for the AsyncRead/AsyncWrite implementation), then that StreamMuxer has to implement Sync.

Again that's a deficiency in Rust.

For example, in this case, the future could technically be Send:

let fut = async move {
    let ptr = std::rc::Rc::new(5);
    let ptr2 = ptr.clone();
};

But in this example, it can't:

let ptr = std::rc::Rc::new(5);
let fut = async move {
    let ptr2 = ptr.clone();
};

But Rust isn't smart enough to figure that out (and it would indeed probably be quite complicated to figure that out).

[Demi-Marie]

@tomaka what if we used a local executor that never migrated futures between threads?

[Demi-Marie]

Sadly, our uses of Rc would look more like the first case than the second. As soon as one needs operate on a StreamMuxer from more than one thread at once, the StreamMuxer must do internal synchronization, which (for current implementations) is slow.

[Demi-Marie]

The standard Linux RDMA library, libibverbs, is used in massive HPC clusters where performance is absolutely critical. It is fully thread-safe ― the same resource can be accessed simultaneously by multiple threads, and atomicity is guaranteed. If they can afford the synchronization overhead, then so can we. Acquiring an uncontended mutex is very, very fast on modern platforms.

Furthermore, calling StreamMuxer methods in multiple threads allows them to do something more clever than just taking a lock. For example, a StreamMuxer might be able to batch encryption and decryption calls, or might queue packets to be encrypted and decrypted asynchronously. None of these optimizations are possible if the synchronization is done externally.

Closing as will not fix.

[thomaseizinger]

As a heads-up, this is now happening: #2765