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

Removed ReceiveBufferSize and SendBufferSize to improve message rates #1414

Closed
wants to merge 1 commit into from

Conversation

MarcoZuiderwijkBS
Copy link
Contributor

Proposed Changes

When connecting our application running in the Western Europe region to a RabbitMQ instance in the East-US region we got a very low message rate (12 messages per second). Message size is around 80kb. We also tested this with a Python (using pika 1.3.2) and a Java consumer (using amqp-client 5.16.0) which had a much higher message rate (> 500 messages per second). We also did a test with RabbitMQ version 5.2.0 and to our surprise this also had a much higher message rate.

My colleague Joey Bleeker investigated the differences between version 5.2.0 and 6.6.0 of the RabbitMQ Client and found this commit. We removed the setting for the ReceiveBufferSize and SendBufferSize. This gives the latest RabbitMQ.Client the same performance as the version 5.2.0. The previous changes seems to conflict with the TCP Window Scaling.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.

@michaelklishin
Copy link
Member

@MarcoZuiderwijkBS thank you for digging in.

As ff627be mentions, some workload may benefit from these settings, while your workload with messages much larger than average suffers.

Sounds like it should be a configurable settings and not something we flip back and forth.

At least several other clients allow you to "post-configure" a socket before it connects using
a lambda. I'd suggest something like that, with the default being "no configuration besides Nagle's algorithm".

@lukebakken lukebakken self-assigned this Nov 15, 2023
@lukebakken lukebakken added this to the 6.7.0 milestone Nov 15, 2023
@lukebakken lukebakken self-requested a review November 15, 2023 16:48
@lukebakken lukebakken marked this pull request as draft November 15, 2023 16:49
@lukebakken
Copy link
Contributor

Converting to draft just to ensure this isn't merged prematurely.

@lukebakken
Copy link
Contributor

lukebakken commented Nov 15, 2023

We should adopt what the Java client does to configure the underlying Socket:

https://github.com/rabbitmq/rabbitmq-java-client/blob/main/src/main/java/com/rabbitmq/client/SocketConfigurator.java

That way, if someone wants to adjust buffer sizes, they can. The only thing we should do is disable Nagle's algo.

cc @stebet on this just so he's in the loop.

@lukebakken
Copy link
Contributor

@MarcoZuiderwijkBS I've merged your code into #1415. Since your PR is on the main branch I can't easily make modifications to it.

@lukebakken lukebakken closed this Nov 15, 2023
@pivotal-cla
Copy link

@MarcoZuiderwijkBS Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@lukebakken
Copy link
Contributor

@pivotal-cla
Copy link

@MarcoZuiderwijkBS Thank you for signing the Contributor License Agreement!

1 similar comment
@pivotal-cla
Copy link

@MarcoZuiderwijkBS Thank you for signing the Contributor License Agreement!

lukebakken added a commit that referenced this pull request Nov 16, 2023
lukebakken added a commit that referenced this pull request Nov 16, 2023
Improve test for SocketFactory

Follow-up to:
* #1414
* #1415
* #1416

https://groups.google.com/g/rabbitmq-users/c/9_ohuUbX9NY
(cherry picked from commit 56bd2c9)
lukebakken added a commit that referenced this pull request Nov 16, 2023
Improve test for SocketFactory

Follow-up to:
* #1414
* #1415
* #1416

https://groups.google.com/g/rabbitmq-users/c/9_ohuUbX9NY
(cherry picked from commit 56bd2c9)
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

4 participants