snapview / tokio-tungstenite

Future-based Tungstenite for Tokio. Lightweight stream-based WebSocket implementation
MIT License
1.88k stars 236 forks source link

Remove flush on each WebSocketStream::poll_ready call + update to tungstenite 0.20 #284

Closed alexheretic closed 1 year ago

alexheretic commented 1 year ago

My context: I've been working on a websocket load generator service that will write lots of small json message events to it's ws clients. I was interested in having a high peak throughput from this service to allow me to test downstream load performance. It uses axum->tokio-tungstenite->tungstenite. Doing localhost testing I got ~1.2M msg/s peak throughput. This PR + https://github.com/snapview/tungstenite-rs/pull/357 improves that to ~2M msg/s.

Currently each SinkExt::feed call will call into tungstenite write_pending on poll_ready meaning flushing before each message write. This is a poor fit for sending lots of small messages where it would be better to be able to feed many messages then flush.

From poll_ready docs:

This method returns Poll::Ready once the underlying sink is ready to receive data.

As tungstenite websockets have a write buffer I believe they are always ready to receive data, and so the impl in this PR simply always returns ready here (without flushing).

Note: Without https://github.com/snapview/tungstenite-rs/pull/357 it actually makes little difference to performance since upstream was also eagerly flushing before each write. So this isn't a big win on it's own. However, if we can agree this impl is more correct I don't see any harm merging before changes upstream.

Update

Now updated to use tungstenite 0.20 (not yet released).

alexheretic commented 1 year ago

I've updated this PR to use the new tungstenite read, write, flush API. And marked it as a draft since tungstenite 0.20 is not yet released.

agalakhov commented 1 year ago

If there is nothing that is broken, we can publish a new release right now. That is, after this work is finished :)

alexheretic commented 1 year ago

To be clear this PR is pretty much ready to go. It'll need an update to replace the dependency on tungstenite git is all.