-
Notifications
You must be signed in to change notification settings - Fork 48
Don't call RegisterType twice for each RPC and with the redis queue it was 4 times! #684
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
Conversation
8c95dd0
to
296247c
Compare
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.
A couple of nits. The typo in the method name being actually somewhat important.
AddProtocolTupesToTypeRegistry(this.typeRegistry); | ||
} | ||
|
||
public static void AddProtocolTupesToTypeRegistry(ITypeRegistry typeRegistry) |
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.
nit: should be AddProtocolTypesToTypeRegistry
(i.e. Types not Tupes)
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.
🤦 sorry yes how did i miss that.
|
||
public static void AddProtocolTupesToTypeRegistry(ITypeRegistry typeRegistry) | ||
{ | ||
foreach (var protocolType in protocolTypes) typeRegistry.RegisterType(protocolType, protocolType.Name, true); |
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.
nit: if we're moving this anyway, let's format it with curly braces across multiple lines.
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.
ok
Background
We would re-register the types each time the serializer was built, since that is built with this lambda each time we serialise or deserialize a message:
With these changes we just register the types once, rather than over and over again.
This as a hot spot was somehow found with a profiler.
With this change we instead call register once for the Request and Response types rather than each time we serialise or deserialise a message (including in and out of the queue). This should have no impact other than improving performance.
How to review this PR
Quality ✔️
Pre-requisites