Getting rid of Command allocations #902
Conversation
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. | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
for consistency reasons should this have the internal keyword?
| { | ||
| using (cmd) | ||
| { | ||
| OnCommandReceived(cmd); | ||
| CommandReceived.Invoke(in cmd); |
There was a problem hiding this comment.
Why not
CommandReceived(in cmd)
?
There was a problem hiding this comment.
I would have prefered
if (cmd.IsEmpty)
{
return;
}
using (cmd)
{
CommandReceived.Invoke(in cmd);
}
There was a problem hiding this comment.
Couldn't this technically be null? Should we always assign a default delegate so that we never need to null check?
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
IntAllocator can be generalised to be a ushort allocator :)
There was a problem hiding this comment.
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.
Proposed Changes
Replaced the Command class with two structs for IncomingCommand and OutgoingCommand.
Types of Changes
Checklist
CONTRIBUTING.mddocumentFurther Comments
Before
After