Performance improvements in the CryptoConfig class. CreateFromName is about 10x faster with this change. #39600

VladimirKhvostov · 2019-07-18T17:29:47Z

CryptoConfig.CreateFromName in the dotnet core is about 4.5 times slower than the same method in .NET

I wrote a simple app:

using System.Diagnostics;
using System.Security.Cryptography;
using System.Threading;
using System.Threading.Tasks;

namespace dotnet
{
    class Program
    {
        static void Main(string[] args)
        {
            Stopwatch stopwatch = Stopwatch.StartNew();
            int threadCount = int.Parse(args[0]);

            int workerThreads;
            int completionPortThreads;
            ThreadPool.GetMinThreads(out workerThreads, out completionPortThreads);

            ThreadPool.SetMinThreads(workerThreads: threadCount, completionPortThreads: completionPortThreads);

            Task[] tasks = new Task[threadCount];

            for (int i = 0; i < threadCount; ++i)
            {
                tasks[i] = Task.Factory.StartNew(() =>
                {
                    for (int n = 0; n < 100_000; ++n)
                    {
                        object o = CryptoConfig.CreateFromName("RSA");
                        Debug.Assert(o != null);
                    }
                });
            }

            Task.WaitAll(tasks);

            Console.WriteLine($"Elapsed time in ms: {stopwatch.Elapsed.TotalMilliseconds:0.###}. Thread count: {threadCount}");
        }
    }
}

My desktop machine has i7-7820X CPU.
Here are results:

Number of threads	.NET 4.8 (time in ms)	.NET core 3.0.0-preview8-27917-01 (time in ms)	NET core 3.0.0 + fix (time in ms)
1	380	81	103
2	460	101	120
4	490	120	130
8	790-980	204-243	157-196
16	3512-3792	535-717	268-270
32	1935-2196	8255-8694	623-633
64	4564-4608	18066-18284	1744-1843

dnfclas · 2019-07-18T17:29:59Z

All CLA requirements met.

VladimirKhvostov · 2019-07-18T17:38:14Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/CryptoConfig.cs

            // Check to see if we have an application defined mapping
-            lock (s_InternalSyncObject)


This lock is really bad and causes contention issues when our web app is running on E20 Azure VMs.
Perfview:
Name

clr!??AwareLock::Contention

clr!JITutil_MonContention
|+ mscorlib.ni!CryptoConfig.CreateFromName
||+ mscorlib.ni!Utils.ObjToHashAlgorithm
|||+ mscorlib.ni!RSACryptoServiceProvider.VerifyData
||| + microsoft.identitymodel.tokens!AsymmetricSignatureProvider.Verify
||| + system.identitymodel.tokens.jwt!JwtSecurityTokenHandler.ValidateSignature
||| |+ system.identitymodel.tokens.jwt!JwtSecurityTokenHandler.ValidateSignature
||| | + system.identitymodel.tokens.jwt!JwtSecurityTokenHandler.ValidateToken
||| | + microsoft.teamfoundation.framework.server!OAuth2AuthenticationService.ValidateToken
||| | + microsoft.teamfoundation.framework.server!OAuth2AuthenticationModule.OnAuthenticateRequest

@sebastienros do we have any benchmark in the ASP.NET Perf lab that uses CryptoConfig.CreateFromName internally?

VladimirKhvostov · 2019-07-18T17:46:27Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/CryptoConfig.cs

@@ -358,6 +347,12 @@ public static object CreateFromName(string name, params object[] args)
                    {
                        retvalType = null;
                    }
+
+                    if (retvalType != null)


This is critical changes that improves perf by about 4-5x.

would it make sense to add this to appNameHT instead and then make DefaultNameHT regular dictionary?

Good idea. I made a change. I see that @adamsitnik implemented a different fix for the issue. See #40184
We might want to take his change instead of this one.

krwq · 2019-07-18T17:57:22Z

@VladimirKhvostov Would it be better to pass HashAlgorithm to RSACryptoServiceProvider.VerifyData instead of string? I suspsect that should give you much better perf results

krwq · 2019-07-18T18:00:27Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/CryptoConfig.cs

-        private static volatile Dictionary<string, Type> appNameHT = new Dictionary<string, Type>(StringComparer.OrdinalIgnoreCase);
-        private static volatile Dictionary<string, string> appOidHT = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+        private static volatile Dictionary<string, string> s_defaultOidHT;
+        private static volatile ConcurrentDictionary<string, object> s_defaultNameHT;


now we will have 3 locks meaning that each of the dictionaries is thread safe but overall state might be torn, @bartonjs is that something we should be concerned about?

s_defaultNameHT is only ever built once (though with this change it looks like now it upgrades itself for delay-loaded type references).

It's actually a bit unfortunate that it needs to be a ConcurrentDictionary at all, but if a major point of this change is to overwrite the strings with Type objects for forward-references then this is the way to go.

But since that upgrade is just a memoization cache on non-user controlled data, there's no state to really be torn, it's "eventually consistent".

sounds good - I'm not familiar with the code, AddAlgorithm here suggested there might be more here than just the cache

jkotas · 2019-07-18T18:14:59Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/CryptoConfig.cs

-        private static volatile Dictionary<string, string> appOidHT = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+        private static volatile Dictionary<string, string> s_defaultOidHT;
+        private static volatile ConcurrentDictionary<string, object> s_defaultNameHT;
+        private static volatile ConcurrentDictionary<string, Type> appNameHT = new ConcurrentDictionary<string, Type>(StringComparer.OrdinalIgnoreCase);


Should these be readonly instead of volatile ?

Seems reasonable to me. I can make the change. I was trying to make minimal changes:)

bartonjs · 2019-07-18T18:35:03Z

@VladimirKhvostov Would it be better to pass HashAlgorithm to RSACryptoServiceProvider.VerifyData instead of string? I suspsect that should give you much better perf results

I sort of agree: the best win here would be to fix microsoft.identitymodel.tokens!AsymmetricSignatureProvider.Verify to use the newer RSA methods on the RSA base class, not the string-or-HashAlgorithm-or-Oid-or-whatever one on RSACryptoServiceProvider.

VladimirKhvostov · 2019-07-18T19:11:56Z

I agree that it would be even better if we could fix the caller. I believe this is the code is here:
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/dev/src/Microsoft.IdentityModel.Tokens/RsaCryptoServiceProviderProxy.cs

Line 203:
We are using earlier version of this library, without the lock around _rsa.VerifyData call - seems like this lock will kill concurrency (unless I am missing something).

        lock (_verifyLock)
        {
            return _rsa.VerifyData(input, hash, signature);
        }

Do you have a recommendation about what needs to be changed in that code (I am from Azure DevOps team - we do not own this library, but I can sync up with maintainers).

At the same time, changing caller will not fix perf regression in this class (compared to full .NET Framework) and will also not fix contention issue, which is in both .NET Framework and .NET core versions.
I figured that since I already spent time looking into this, it makes sense to improve perf the the greater good.

VladimirKhvostov · 2019-07-18T19:12:28Z

Sorry. I clicked on Close Comment - I am new to GitHub. I am using Azure DevOps most of the time.

bartonjs · 2019-07-18T19:16:53Z

- return _rsa.VerifyData(input, hash, signature);
+ return _rsa.VerifyData(input, signature, theCorrectHashAlgorithmNameValue, RSASignaturePadding.Pkcs1);

GSPP

Why does s_defaultNameHT need to be concurrent? It seems it is only written to before being published.

maryamariyan · 2019-08-08T08:26:23Z

@VladimirKhvostov thank you for your PR. we are currently triaging open PRs on corefx. Seems like this one wasn't active for a couple of weeks.
There seems to be some outstanding tasks here. There seems to be a conflict on your latest commit.
Do you think you would be making updates to it soon or prefer to close and reopen later?

adamsitnik · 2019-08-08T10:56:40Z

Why does s_defaultNameHT need to be concurrent? It seems it is only written to before being published.

The s_defaultNameHT is exposed via DefaultNameHT property which can be used to modify the content of the dictionary:

corefx/src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/CryptoConfig.cs

Line 360 in 74acb64

DefaultNameHT[name] = retvalType;

…olatile. Made a change to specify initial capacity when creating DefaultOidHT and DefaultNameHT dictionaries.

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/CryptoConfig.cs

krwq · 2019-08-08T20:57:50Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/CryptoConfig.cs

-                {
-                    appNameHT[name] = algorithm;
-                }
+                appNameHT[name] = algorithm;


lock inside of the loop is I guess fine since this is called relatively rarely and presumably once per process

This method should not be called very often, but someone can write the code to call it on every request.
The reason I removed lock here is because I really needed to remove lock in the CreateFromName method, which causes contention. See line 336.

krwq · 2019-08-08T21:17:58Z

Generally looks good to me, one comment: #39600 (comment)

…ctionary now.

krwq

See offline thread

krwq

We need to figure out if we want to merge some of the @adamsitnik's stuff into here or reverse

wangzq · 2020-06-18T21:57:20Z

Will this get backported to .NET? I am still seeing this issue in mscorlib 4.8.4180.0; I am referring to following lock contention

internal static string MapNameToOID(string name, OidGroup oidGroup)
{
	if (name == null)
	{
		throw new ArgumentNullException("name");
	}
	InitializeConfigInfo();
	string text = null;
	lock (InternalSyncObject)
	{
		text = appOidHT.GetValueOrDefault(name);
	}

bartonjs · 2020-06-18T22:09:28Z

@wangzq

Will this get backported to .NET [Framework]?

No. .NET Framework is only receiving critical fixes at this point (https://devblogs.microsoft.com/dotnet/net-core-is-the-future-of-net/).

If lock contention in MapNameToOID is showing up for you, my recommendation would be to try to eliminate calls to it, if you can. For example, instead of RSACryptoServiceProvider.VerifyData(byte[], object, byte[]), call RSA.VerifyData(byte[], byte[], HashAlgorithmName, RSASignaturePadding)

wangzq · 2020-06-18T22:17:36Z

@bartonjs Thanks for quick response; currently this is being triggered from System.IdentityModel:

mscorlib.dll!System.Security.Cryptography.CryptoConfig.MapNameToOID(string name, System.Security.Cryptography.X509Certificates.OidGroup oidGroup) (IL=0x0025, Native=0x00007FFD3B935DF0+0xB4)
 	mscorlib.dll!System.Security.Cryptography.RSAPKCS1SignatureDeformatter.SetHashAlgorithm(string strName) (IL≈0x0000, Native=0x00007FFD3C418A20+0x40)
 	mscorlib.dll!System.Security.Cryptography.RSAPKCS1SignatureDescription.CreateDeformatter(System.Security.Cryptography.AsymmetricAlgorithm key) (IL=0x0014, Native=0x00007FFD3C4187D0+0x5D)
 	System.IdentityModel.dll!System.IdentityModel.Tokens.X509AsymmetricSecurityKey.GetSignatureDeformatter(string algorithm) (IL≈0x004A, Native=0x00007FFD3C417A00+0x19C)
 	System.IdentityModel.Tokens.Jwt.dll!System.IdentityModel.Tokens.AsymmetricSignatureProvider.AsymmetricSignatureProvider(System.IdentityModel.Tokens.AsymmetricSecurityKey key, string algorithm, bool willCreateSignatures) (IL≈0x0248, Native=0x00007FFD3D5662B0+0x93E)
 	System.IdentityModel.Tokens.Jwt.dll!System.IdentityModel.Tokens.SignatureProviderFactory.CreateProvider(System.IdentityModel.Tokens.SecurityKey key, string algorithm, bool willCreateSignatures) (IL=0x0105, Native=0x00007FFD3D5657A0+0x62E)
 	System.IdentityModel.Tokens.Jwt.dll!System.IdentityModel.Tokens.SignatureProviderFactory.CreateForVerifying(System.IdentityModel.Tokens.SecurityKey key, string algorithm) (IL≈0x0000, Native=0x00007FFD3D565740+0x3A)
 	System.IdentityModel.Tokens.Jwt.dll!System.IdentityModel.Tokens.JwtSecurityTokenHandler.ValidateSignature(byte[] encodedBytes, byte[] signature, System.IdentityModel.Tokens.SecurityKey key, string algorithm) (IL≈0x0006, Native=0x00007FFD3D565410+0x6D)
 	System.IdentityModel.Tokens.Jwt.dll!System.IdentityModel.Tokens.JwtSecurityTokenHandler.ValidateSignature(string token, System.IdentityModel.Tokens.TokenValidationParameters validationParameters) (IL≈0x0173, Native=0x00007FFD3D562B70+0x775)
 	System.IdentityModel.Tokens.Jwt.dll!System.IdentityModel.Tokens.JwtSecurityTokenHandler.ValidateToken(string securityToken, System.IdentityModel.Tokens.TokenValidationParameters validationParameters, out System.IdentityModel.Tokens.SecurityToken validatedToken) (IL≈0x006C, Native=0x00007FFD3D562040+0x2B9)

VladimirKhvostov commented Jul 18, 2019

View reviewed changes

danmoseley requested review from bartonjs and krwq July 18, 2019 17:40

VladimirKhvostov commented Jul 18, 2019

View reviewed changes

krwq reviewed Jul 18, 2019

View reviewed changes

jkotas reviewed Jul 18, 2019

View reviewed changes

VladimirKhvostov closed this Jul 18, 2019

VladimirKhvostov reopened this Jul 18, 2019

VladimirKhvostov force-pushed the CryptoConfigPerfImprovements branch from b742ee3 to 74acb64 Compare July 19, 2019 19:27

adamsitnik added the tenet-performance Performance related issue label Jul 31, 2019

adamsitnik mentioned this pull request Jul 31, 2019

CryptoConfig.CreateFromName benchmarks dotnet/performance#718

Merged

karelz added this to the 5.0 milestone Aug 3, 2019

GSPP reviewed Aug 7, 2019

View reviewed changes

eiriktsarpalis added the area-System.Security label Aug 8, 2019

eiriktsarpalis assigned bartonjs Aug 8, 2019

Vladimir Khvostov added 2 commits August 8, 2019 10:15

Performance improvements in the CryptoConfig class.

90ebb00

PR feedback - declare appNameHT and appOidHT as readonly instead of v…

11d8f1a

…olatile. Made a change to specify initial capacity when creating DefaultOidHT and DefaultNameHT dictionaries.

VladimirKhvostov force-pushed the CryptoConfigPerfImprovements branch from 74acb64 to 11d8f1a Compare August 8, 2019 14:36

krwq self-assigned this Aug 8, 2019

krwq reviewed Aug 8, 2019

View reviewed changes

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/CryptoConfig.cs Outdated Show resolved Hide resolved

krwq reviewed Aug 8, 2019

View reviewed changes

adamsitnik mentioned this pull request Aug 9, 2019

Improve CryptoConfig.CreateFromName performance (from 10% to x20) #40184

Closed

PR feedback - s_defaultNameHT is a Dictionary instead of ConcurrentDi…

e60f778

…ctionary now.

krwq suggested changes Aug 9, 2019

View reviewed changes

krwq approved these changes Aug 9, 2019

View reviewed changes

bartonjs approved these changes Aug 13, 2019

View reviewed changes

bartonjs merged commit 963e0d6 into dotnet:master Aug 13, 2019

VladimirKhvostov mentioned this pull request Feb 1, 2020

CryptoConfig.CreateFromName in dotnet core is about 4.5 times slower than the same method in .NET Framework dotnet/runtime#30299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance improvements in the CryptoConfig class. CreateFromName is about 10x faster with this change. #39600

Performance improvements in the CryptoConfig class. CreateFromName is about 10x faster with this change. #39600

VladimirKhvostov commented Jul 18, 2019

dnfclas commented Jul 18, 2019 •

edited

Loading

VladimirKhvostov Jul 18, 2019

adamsitnik Jul 31, 2019

VladimirKhvostov Jul 18, 2019

krwq Aug 8, 2019

VladimirKhvostov Aug 9, 2019

krwq commented Jul 18, 2019 •

edited

Loading

krwq Jul 18, 2019

bartonjs Jul 18, 2019

krwq Jul 18, 2019

jkotas Jul 18, 2019

VladimirKhvostov Jul 18, 2019

bartonjs commented Jul 18, 2019

VladimirKhvostov commented Jul 18, 2019

VladimirKhvostov commented Jul 18, 2019 •

edited

Loading

bartonjs commented Jul 18, 2019

GSPP left a comment

maryamariyan commented Aug 8, 2019

adamsitnik commented Aug 8, 2019

krwq Aug 8, 2019

VladimirKhvostov Aug 9, 2019

krwq commented Aug 8, 2019

krwq left a comment

krwq left a comment

wangzq commented Jun 18, 2020 •

edited

Loading

bartonjs commented Jun 18, 2020

wangzq commented Jun 18, 2020

		// Check to see if we have an application defined mapping
		lock (s_InternalSyncObject)

Performance improvements in the CryptoConfig class. CreateFromName is about 10x faster with this change. #39600

Performance improvements in the CryptoConfig class. CreateFromName is about 10x faster with this change. #39600

Conversation

VladimirKhvostov commented Jul 18, 2019

dnfclas commented Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

This lock is really bad and causes contention issues when our web app is running on E20 Azure VMs. Perfview: Name

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Jul 18, 2019

VladimirKhvostov commented Jul 18, 2019

VladimirKhvostov commented Jul 18, 2019 • edited Loading

bartonjs commented Jul 18, 2019

GSPP left a comment

Choose a reason for hiding this comment

maryamariyan commented Aug 8, 2019

adamsitnik commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Aug 8, 2019

krwq left a comment

Choose a reason for hiding this comment

krwq left a comment

Choose a reason for hiding this comment

wangzq commented Jun 18, 2020 • edited Loading

bartonjs commented Jun 18, 2020

wangzq commented Jun 18, 2020

dnfclas commented Jul 18, 2019 •

edited

Loading

This lock is really bad and causes contention issues when our web app is running on E20 Azure VMs.
Perfview:
Name

krwq commented Jul 18, 2019 •

edited

Loading

VladimirKhvostov commented Jul 18, 2019 •

edited

Loading

wangzq commented Jun 18, 2020 •

edited

Loading