Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 2, 2023

Conversation

alexheretic
Copy link
Contributor

@alexheretic alexheretic commented May 24, 2023

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 + snapview/tungstenite-rs#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 snapview/tungstenite-rs#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 alexheretic marked this pull request as ready for review May 24, 2023 17:36
@alexheretic alexheretic marked this pull request as draft May 28, 2023 18:30
@alexheretic alexheretic changed the title Remove flush on each WebSocketStream::poll_ready call Remove flush on each WebSocketStream::poll_ready call + update to tungstenite 0.20 May 28, 2023
@alexheretic
Copy link
Contributor Author

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
Copy link
Member

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

@alexheretic alexheretic marked this pull request as ready for review May 29, 2023 00:25
@alexheretic
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants