Don't allocate method name strings in protocol#41465
Conversation
- Today SignalR allocates a string when parsing the incoming protocol message, this change introduces a new API on IInvocationBinder that lets us avoid that allocation by asking the invocation provider for the string. - This can be implemented on the client as well, that's not in this PR.
src/SignalR/common/Protocols.Json/src/Protocol/JsonHubProtocol.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Co-authored-by: Brennan <brecon@microsoft.com>
|
|
||
| internal void Add(string value) | ||
| { | ||
| var hashCode = GetKeyHashCode(value.AsSpan()); |
There was a problem hiding this comment.
Can we do the UTF8 encoding here during initialization once per method instead of for every invocation in TryGetValue? Case-insensitive comparisons might be harder this way, but could we just check for ordinal equality. How common is it really to use different casing for the method names on the server? A cache miss costs an allocation, but the upside is saving the encoding step in the common case. I know the encoding is usually just a simple widen in practice, but it seems worth it to avoid it if we can.
There was a problem hiding this comment.
Not worth it. The performance isn't bad enough to spend time on this.
I will add that I started by converting to utf8 and case insensitivity lead me to revert back to utf16. When using the javascript clients its common to not match the casing on the server and all of the efficient methods for lower case sensitive comparison is utf16 based.
| internal int hashCode; | ||
| internal int next; | ||
| internal string key; | ||
| internal string value; |
There was a problem hiding this comment.
Are key and value ever different?
There was a problem hiding this comment.
I don't think they are different:
Is this getting reassigned anywhere? I don't see it.
There was a problem hiding this comment.
Ah yes, I can remove that. I used to ToLowerInvariant the key when I made the initial change.
|
@davidfowl I wish I noticed you enabled auto merge 😞. Not a huge deal, but are you going to submit a follow up? |
|
I have no plans to further optimize this no 😄 |
Performance looks good:
Code here:
Details
Fixes #41342