-
Notifications
You must be signed in to change notification settings - Fork 591
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
Conversation
There was a problem hiding this 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.
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.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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)
?
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
Proposed Changes
Replaced the Command class with two structs for IncomingCommand and OutgoingCommand.
Types of Changes
Checklist
CONTRIBUTING.md
documentFurther Comments
Before
After