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

Inefficient use of buffers reduces potential throughput for basicPublish with small messages. #141

Closed
fadams opened this issue Dec 18, 2022 · 4 comments
Assignees
Milestone

Comments

@fadams
Copy link
Contributor

fadams commented Dec 18, 2022

I've recently started experimenting with amqp091-go and I had in the back of my mind some past experience with AMQP in Rust, so I put together a very simple producer https://gist.github.com/fadams/28ddeb912d1421b01b6d4591cda62290 based on https://github.com/rabbitmq/rabbitmq-tutorials/blob/main/go/send.go that publishes a large number of relatively small messages in a loop. The performance was OK, but was around half the throughput of the roughly equivalent Rust version which didn't seem right.

I ended up profiling with pprof and noted that Syscall was the hottest point, so started looking at publishWithDeferredConfirmWithContenxt then send then sendOpen on Channel. What I noticed was that sendOpen in Channel does multiple send calls on Connection (methodFrame, headerFrame, N x bodyFrame), however the send on Connection calls WriteFrame on writer, but that method both writes to the buffer and flushes.

Flushing the write buffer for every Frame largely defeats the point of using a buffered writer, certainly for the case of small messages, and it is significantly more efficient to write the message by writing each Frame to the buffer then flushing the complete message.

This patch https://gist.github.com/fadams/7ab99557c67852121bbe4f1d8dcd2ee2 adds sendUnflushed() and flush() methods to Connection and replaces the ch.connection.send calls in Channel sendOpen with ch.connection.sendUnflushed and ends with an explicit ch.connection.flush call. The patch also moves the notifier used to reduce heartbeats to the flush method because all the frames in message are semantically related and there is no real advantage to sending per Frame vice per "group of related Frames" and for the case of small messages time.Now() is (relatively) expensive.

With this patch the observed throughput for small messages is around double the original observed throughput.

amqp091-go-patches.tar.gz

@lukebakken
Copy link
Contributor

Please feel free to open a pull request. Thanks.

@fadams
Copy link
Contributor Author

fadams commented Dec 19, 2022

Done:
#142

@lukebakken lukebakken self-assigned this Dec 19, 2022
@lukebakken lukebakken added this to the 1.6.0 milestone Dec 19, 2022
@Zerpet
Copy link
Contributor

Zerpet commented Dec 21, 2022

Merged #142. Thank you!

@Zerpet Zerpet closed this as completed Dec 21, 2022
@fadams
Copy link
Contributor Author

fadams commented Dec 21, 2022

You're very welcome. That's great. Thanks for picking this up so quickly.

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

No branches or pull requests

3 participants