Improve CryptoConfig.CreateFromName performance (from 10% to x20)#40184
Improve CryptoConfig.CreateFromName performance (from 10% to x20)#40184adamsitnik wants to merge 6 commits intodotnet:masterfrom
Conversation
|
@adamsitnik, how do your changes relate to #39600? I've not reviewed yours, but from the description it seems duplicative. |
|
Are any real-world E2E scenarios improved by this change? I'd be surprised to learn that this function is the bottleneck in any legitimate use of cryptography. |
|
@GrabYourPitchforks this method is used in the JWT token validation code path, which caused several LSIs in other DevOps. See #39600 for details. |
|
@VladimirKhvostov I saw that, but it seems the conclusion from that thread was to change the JWT verification implementation to call a different method. To an extent this matches recommended practice: the CryptoConfig class is really intended for use only when you need agility. JWT verification shouldn't be bouncing through this class because the JWT validation library should be hardcoding the list of supported algorithms; it doesn't require the type of agility the CryptoConfig class is intended to provide. |
|
cc @krwq |
|
@adamsitnik thanks! I think next steps would be to not call CryptoConfig.CreateFromName at all and just use algorithm directly. That would avoid any lookup at all |
|
|
||
| string[] algorithmNames = new string[names.Length]; | ||
| Array.Copy(names, 0, algorithmNames, 0, algorithmNames.Length); | ||
| string[] safeCopy = names.Length == 1 ? new string[1] { names[0] } : names.ToArray(); |
There was a problem hiding this comment.
Does new string[1] { names[0] } actually make that difference? If so, could we perhaps improve ToArray instead?
There was a problem hiding this comment.
I suspect CryptoConfig.AddAlgorithm most often called with 1 argument. We can either special case this - not create safeCopy if names.Length == 1 or even create an overload AddAlgorithm, which would take string name instead string names[].
There was a problem hiding this comment.
This is definitely not worth extra code/complexity. Just do:
string[] algorithmNAmes = names.AsSpan().ToArray();It's simpler and its perf will be just fine; I cannot believe any perf difference will be measurable in real-world usage, especially considering we're about to do cryptography!
(If you really believed Length == 1 was special and warranted optimization, you'd want to special case everything for the rest of the method, too; there's no point allocating another array, looping with foreach twice, etc., when you can just operate on the sole value. But, again, I sincerely doubt it's worth the extra complexity.)
| typeNameToType["http://www.w3.org/2001/04/xmlenc#sha512"] = SHA512DefaultType; | ||
|
|
||
|
|
||
| typeNameToType["http://www.w3.org/2001/04/xmlenc#aes128-cbc"] = RijndaelManagedType; |
There was a problem hiding this comment.
@bartonjs should we be using AesManagedType directly here?
|
I have some security concerns about this fix (and also original PR) - will need to discuss it with @bartonjs since he's more familiar with how this is being usage. I'll take this offline |
| { | ||
| ConcurrentDictionary<string, Type> typeNameToType = new ConcurrentDictionary<string, Type>(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| Type HMACMD5Type = typeof(System.Security.Cryptography.HMACMD5); |
There was a problem hiding this comment.
It is better to get the type right next to its use. Fetching all types first into locals will result in larger stack frame and bigger native code.
|
#39600 has been accepted. If there're any more gain to be gotten from this PR it would need to be re-evaluated in terms of the new baseline. Since it'd be a different change with different benchmark justification, I'm closing this PR. |
I was working on making it easier for CoreFX team members to use ASP.NET Perf Lab to run performance tests under high load.
Then I saw #39600 which was a perfect candidate for sample for the docs: we had some nasty locks that were making the perf bad. So I created sample docs based on this example: aspnet/Benchmarks#1284
After taking a brief look at the existing code, I could not resist rewriting it and make it even faster than @VladimirKhvostov did in #39600
ASP.NET Perf lab test result (described in aspnet/Benchmarks#1284):
Before:
After:
Some microbenchmark results:
@danmosemsft this was one of the cases where Full .NET Framework was faster than Core. I made sure that Core is faster now.
The key changes:
Type.GetType(string name)is very expensive. Every type is now loaded once and then saved to the dictionary. This was the biggest improvement