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

Rework as Bytes-only payload #473

Merged
merged 10 commits into from
Dec 17, 2024

Conversation

alexheretic
Copy link
Contributor

@alexheretic alexheretic commented Dec 17, 2024

Spurred by the chat in tokio-rs/axum#3078 This PR experiments with simplifying the API to use only Bytes as payload. It does work pretty well, the only downside seems to be client->server write performance as masking requires mutating the bytes. Now fixed.

  • Read unmask performance was addressed by unmasking before converting to Bytes to avoid the overhead of having to mutate them.
  • Write masking performance was addressed by writing and unmasking in the out_buffer rather than in-place.

@alexheretic
Copy link
Contributor Author

I wonder if there is a way of optimising the write to not need to convert Bytes too...

@agalakhov
Copy link
Member

Performance regression at 100k small messages seems to be significant. This however does not mean that it will be so in real use. I would test it a little bit more in other environments. The real question is, how many Arcs exist in real life and how many of them do exist only due to Bytes.

Such a performance impact (10%) in a real-life high load example would be huge. Ten percent more load means ten percent more servers in the cluster and ten percent more hardware costs, and hardware is expensive.

@alexheretic
Copy link
Contributor Author

Actually I found we don't need to apply the mask then write, we have an out_buffer so we can write into the out_buffer then apply the mask to the written slice. This has no extra cost and means we have no need to mutate the payloads at any point.

@alexheretic
Copy link
Contributor Author

Write performance looks good now

write 100k small messages then flush (server)
                        time:   [4.7412 ms 4.7432 ms 4.7437 ms]
                        change: [+2.1394% +2.4145% +2.6909%] (p = 0.08 > 0.05)

write+mask 100k small messages then flush (client)
                        time:   [5.9248 ms 5.9282 ms 5.9416 ms]
                        change: [-1.9907% -1.5423% -1.0910%] (p = 0.05 < 0.05)

@daniel-abramov daniel-abramov merged commit 99bef17 into snapview:master Dec 17, 2024
7 checks passed
@alexheretic alexheretic deleted the bytes-only-payload branch December 17, 2024 16:48
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.

4 participants