-
Couldn't load subscription status.
- Fork 489
Performance Improvements in SIP Authorization Digest Parsing #1338
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
base: master
Are you sure you want to change the base?
Conversation
| { | ||
| //authRequest.Algorithm = headerValue; | ||
|
|
||
| #if NETCOREAPP6_0_OR_GREATER |
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.
Is this preprocessor define correct? It's not lsied on https://learn.microsoft.com/en-us/dotnet/standard/frameworks#preprocessor-symbols.
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.
You're right and that's a nasty bug hidden under there.
Are the - valid anywhere, or are there specific strings that should be accepted? I was looking at this and thinking a dictionary would be a better fit than parsing modified text.
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.
The '-' are only being replaced because they can't be used in C# enums. It's safe to do so for the AUTH_ALGORITHM_KEY values. If the algorithm is not in the enum it means it's not recognised and the digest won't be able to be verified/generated any way,
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.
Other than MD5 and SHA256 what other values are to be expected? SHA-256?
I wouldn't expect -M-D-5- to be valid. Is it?
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.
MD5
SHA-256
SHA-512 (not wired up, although would be trivial)
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.
Is SHA256 a valid value? Is it case insensitive?
ce59676 to
518a84b
Compare
7af6518 to
b0ee042
Compare
42a5bb1 to
255a826
Compare
12944e6 to
01482ca
Compare
…cs` to use `ReadOnlySpan<char>` for improved performance and memory efficiency. Simplified header field handling by replacing regular expressions and string arrays with spans and slices. Expanded unit tests in `SIPAuthorisationDigestUnitTest.cs` to cover various scenarios, including basic, full, malformed, and case-insensitive headers, as well as nonce counts and QOP values.
|
This looks like a good optimization effort! Would you be able to share any benchmark data or profiling results that illustrate the performance gains? Seeing the before-and-after numbers would be really helpful, as I’m always keen to learn more about identifying and resolving performance bottlenecks in SIP processing. I'm also curious how the regex would perform if they were to be made source generated. |
Sure! I've applied well known and established practices, but I can provide a benchmark. Can you provide example Sometimes these things only shine up in production with other components in the same process. But after a few years, you learn to spot them. |
Performance Improvements in SIP Authorization Digest Parsing
This PR introduces significant performance and memory optimizations to the
ParseAuthorisationDigestmethod by leveragingSpan<T>and eliminating unnecessary string allocations and regular expressions.Key Improvements
1. Elimination of Regular Expression Operations
Regex.Matchcalls that were being performed for each header field comparisonEqualswithStringComparison.OrdinalIgnoreCase2. Memory Usage Optimization
ReadOnlySpan<T>to avoid string allocationsSpan<T>slicing operations instead ofString.Substring()3. More Efficient Header Parsing
string.Split()Trim()on spans4. Cross-Platform Optimizations
Performance Impact
Testing Notes
This change maintains the exact same functionality while improving performance. All existing tests should pass without modification.
Resolves issues related to: