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

Frame optimizations #844

Merged
merged 3 commits into from
May 25, 2020
Merged

Conversation

stebet
Copy link
Contributor

@stebet stebet commented May 21, 2020

Proposed Changes

  • Buffers outbound frames using a Channel, and create a background task that consumes from that channel and writes to the socket. This makes for more efficient buffering of outbound frames as well as removes that write-lock from the SocketFrameHandler since there is only ever one writer accessing the socket.
  • As a result, we also get rid of a temporary list allocation when writing frame sets, and we can simplify the command->frame splitting code as well as simplify the socket writers.

Types of Changes

  • 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

  • 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

@stebet
Copy link
Contributor Author

stebet commented May 21, 2020

Memory usage before:
Before

Memory usage after:
After

CPU usage before:
CPUBefore

CPU usage after:
CPUAfter

@stebet
Copy link
Contributor Author

stebet commented May 22, 2020

One failed test... taking a look.

@stebet
Copy link
Contributor Author

stebet commented May 22, 2020

Ahhh, the test is broken :) It's failing to copy the buffers of the received messages, and is therefore breaking our new rule in 6.0 to make sure to copy the contents of the message body in the event handlers.

@lukebakken lukebakken requested review from lukebakken and michaelklishin and removed request for lukebakken May 22, 2020 13:59
@lukebakken lukebakken self-assigned this May 22, 2020
@lukebakken lukebakken added this to the 6.1.0 milestone May 22, 2020
@michaelklishin
Copy link
Member

@stebet please rebase this on top of master. Thank you.

stebet added 3 commits May 25, 2020 14:29
Using a Channel to buffer outbound frames, moving socket writes to a background task and getting rid of the streamlock since there is only ever one writer accessing it.
… event body since it didn't copy the contents of the buffers.
@stebet
Copy link
Contributor Author

stebet commented May 25, 2020

Arrghh.... think I royally messed up this rebase...

@stebet stebet force-pushed the frameOptimizations branch from db4e926 to 33d83e7 Compare May 25, 2020 14:36
@stebet
Copy link
Contributor Author

stebet commented May 25, 2020

Ok, think I managed to fix it :)

@michaelklishin michaelklishin merged commit ed3b780 into rabbitmq:master May 25, 2020
@michaelklishin
Copy link
Member

@stebet should I backport this?

@stebet
Copy link
Contributor Author

stebet commented May 25, 2020

Should be safe. @bording ?

@bording
Copy link
Collaborator

bording commented May 25, 2020

I think the rebase was still not right, because it looks like this is PR changed the site submodule, which I wouldn't expect this PR to touch.

Should be safe. @bording ?

Yeah I think this would be okay to bring over to 6.1.0. There's no behavior or public API changes.

michaelklishin added a commit that referenced this pull request May 25, 2020
Frame optimizations

(cherry picked from commit ed3b780)
@michaelklishin
Copy link
Member

Backported to 6.x.

@stebet
Copy link
Contributor Author

stebet commented May 25, 2020

The submodule keeps giving me a headache, and sneaking in there. Is there a way to ignore it automatically?

@bording
Copy link
Collaborator

bording commented May 25, 2020

@michaelklishin I think you'll need to fix the submodule on master and 6.x

@michaelklishin
Copy link
Member

michaelklishin commented May 25, 2020 via email

@bollhals bollhals mentioned this pull request May 31, 2020
11 tasks
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