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

Getting rid of Command allocations #902

Merged
merged 2 commits into from
Jul 8, 2020

Conversation

bollhals
Copy link
Contributor

@bollhals bollhals commented Jul 7, 2020

Proposed Changes

Replaced the Command class with two structs for IncomingCommand and OutgoingCommand.

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

Further Comments

Before
image
After
image

Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, another 8% reduction in memory footprint.

@michaelklishin michaelklishin merged commit 8778414 into rabbitmq:master Jul 8, 2020
michaelklishin added a commit that referenced this pull request Jul 8, 2020
Getting rid of Command allocations

(cherry picked from commit 8778414)
@@ -608,7 608,7 @@ public void HandleBasicCancelOk(string consumerTag)
_consumers.Remove(consumerTag);
}
ConsumerDispatcher.HandleBasicCancelOk(k.m_consumer, consumerTag);
k.HandleCommand(null); // release the continuation.
k.HandleCommand(IncomingCommand.Empty); // release the continuation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to imply anything needs to be changed. I'm asking for my own benefit.

Why .Empty instead of just default? My preference would have been default because I don't think .Empty conveys much especially given .Empty is actually default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I didn't think too much about it. I do it for all "empty instances", but this one is actually a struct, so I'm uncertain about the consequences here. I'll need to investigate but my assumption is that default will (apart from the visibility) even be better overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm interesting. My benchmark suggests that the Empty is faster here. Probably it is due to the fact of the in of the HandleCommand. The default case will have to create a temporary local to pass in and thus zeroing the stack for the "big" struct. Where as in the Empty case, the method just takes a reference of the static Empty field.

@@ -44,12 44,14 @@

namespace RabbitMQ.Client.Impl
{
interface ISession
delegate void CommandReceivedAction(in IncomingCommand cmd);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency reasons should this have the internal keyword?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good catch

{
using (cmd)
{
OnCommandReceived(cmd);
CommandReceived.Invoke(in cmd);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not

CommandReceived(in cmd)

?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have prefered

            if (cmd.IsEmpty)
            {
                return;
            }

            using (cmd)
            {
                CommandReceived.Invoke(in cmd);
            }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this technically be null? Should we always assign a default delegate so that we never need to null check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my style, I don't mind adapting to whatever style there is preferred here.

Couldn't this technically be null? Should we always assign a default delegate so that we never need to null check?

technically yes, as it's not set in the constructor, but it's done in Session.Initialise which is called from the constructor. In that sense no it can not.

@@ -99,7 99,7 @@ public ISession CreateInternal(int channelNumber)
{
lock (_sessionMap)
{
ISession session = new Session(_connection, channelNumber);
ISession session = new Session(_connection, (ushort)channelNumber);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are now casting to ushort wouldn't it make sense to go one step further to change the SessionManager, IntAllocator and the rest of the infra as well to get rid of the cast?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IntAllocator can be generalised to be a ushort allocator :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's possible. I didn't want to change too much in one go.
Also I think there is some public API that exposes it as int, so this is most likely going to stay.

lukebakken added a commit that referenced this pull request Jul 9, 2020
@bollhals bollhals deleted the eliminate.command branch March 2, 2021 20:50
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.

3 participants