-
Notifications
You must be signed in to change notification settings - Fork 700
Poor query performance in IdentityServer4 3.1.4 #4672
Comments
Did you revert the change and test it with our v4.0.x? That's where we'd consider making this change. Also, can you share anything on how you did the testing (test scripts, etc)? I'd like to be able to run the same tests locally to confirm. I am a bit boggled -- the reason for the prior change was to address the per issues introduced by changes in EFCore for 3.x. So now you're suggesting we revert those. So I'm trying to figure out which code style w/ EF is the real perf problem. Thanks. |
I haven't tested it on v4.0.x as it introduced a lot of breaking changes that will be expensive for me to move over for now. The implementation I pasted is running on IdentityServer4 3.1.4 towards a Microsoft SQL Server 2016 SP2. The issue in #3718 is in combination with Postgres, it might be that Postgres caches data differently compared to Microsoft SQL. I am not suggesting we should revert the change, but rather investigate why we are seeing this difference between Postgres and Microsoft SQL. Just because the code I posted above works better for my scenario doesn't mean it works for everyone.
|
I did some more testing with the different changes that has been made to the |
Would you be willing to try Postgres to see how that compares? |
I will do some investigation and see if I can replicate that behavior with Postgresql. |
I've set up a local SQL server with MSSQL and Postgresql and ran the Artillery script I have above (without the introspection requests), that resulted in the following results: Postgresql: By unoptimized I mean the code that IdentityServer4 comes with out of the box, while optimized is with the change I have commented with in this issue. In other words, the change actually decreases performance for both Postgresql and MSSQL. |
Ok, thanks for that research. We'll figure out what to do next. |
I managed to replicate the issue in #3718 with more data in my configuration tables. As the issue they experienced was due to the cartesian product, I got the following results towards MSSQL by adding 40 ClientRedirectUris to the client that I have: Will do some more digging and see if I can find a way to optimize this that benefits both use-cases. What is most interesting is how the response time jumped up from the old IdentityServer 2.1.1 to 3.x even with running the queries separately. |
I ended up with the following implementation of public override async Task<Client> FindClientByIdAsync(string clientId)
{
var baseQuery = Context.Clients.Where(x => x.ClientId == clientId);
var client = await baseQuery.FirstOrDefaultAsync();
await baseQuery.SelectMany(x => x.AllowedCorsOrigins).LoadAsync();
await baseQuery.SelectMany(x => x.AllowedGrantTypes).LoadAsync();
await baseQuery.SelectMany(x => x.AllowedScopes).LoadAsync();
await baseQuery.SelectMany(x => x.Claims).LoadAsync();
await baseQuery.SelectMany(x => x.ClientSecrets).LoadAsync();
await baseQuery.SelectMany(x => x.IdentityProviderRestrictions).LoadAsync();
await baseQuery.SelectMany(x => x.PostLogoutRedirectUris).LoadAsync();
await baseQuery.SelectMany(x => x.Properties).LoadAsync();
await baseQuery.SelectMany(x => x.RedirectUris).LoadAsync();
var model = client?.ToModel();
Logger.LogDebug("{clientId} found in database: {clientIdFound}", clientId, model != null);
return model;
} This nearly halfed the response times from a baseline on 118ms. Unfortunately I wasn't able to reproduce the high response times as my friends had done maintenance on the database server while I was on vacation. However, I have noticed that with this change I get more or less identical performance as with the 2.x of IdentityServer4. Comparing the queries, I see that |
I have completed performing benchmarks with async vs. non-async and found that using async APIs caused a very unstable response times around 60-100. With non-async APIs I got a stable 30ms response time which is faster than 2.x. |
@ajcvickers any ideas (perhaps on this whole thread)? |
From what I gather, with async the API will basically queue up work for the database to process, as the API will start processing next request once it is waiting for the SQL server to do IO. What I am seeing is that the database is struggling with keeping up with processing the queries generated by the API. This discussion is related to #2067. I also found several queries that can be optimized and fine-tuned in the stores. For example, getting an entry from PersistedGrants is using a regular .Where, which runs a .ToArrayAsync and then it runs a .SingleOrDefault with the same predicate as in the .Where: var persistedGrant = (await Context.PersistedGrants.AsNoTracking().Where(x => x.Key == key).ToArrayAsync())
.SingleOrDefault(x => x.Key == key); This can be simplified to: var persistedGrant = await Context.PersistedGrants
.AsNoTracking()
.FirstOrDefaultAsync(x => x.Key == key); The same can be seen in |
Did some digging today and found that the following stack trace occurred several times in the process dumps I took under heavy load:
To me, this looks like a synchronous API-call inside EF-Core internals towards the Microsoft.Data.SqlClient using its own synchronous This looks like have been fixed by a later commit: dotnet/efcore@1e977ae#diff-c5e724774bdd22fa0e9f3c2143e74d1b I will go ahead and create an issue on their gh regarding this. |
/cc @roji since we have been discussing cases like this. |
Some comments:
|
I have ended up mitigating this issue by using the in-memory cache for ClientStore and ResourceStore for now. This will effectively eliminate almost all SELECT queries (except for the ones towards PersistedGrants). For my case it seems that the performance issues I experience is a combination of periodically slow SQL server together with excessive use of async and possibly blocking APIs. I have not been able to reproduce the perf issues with pgsql so far. |
@martinmine thanks for the additional details. AFAIK with Npgsql/PostgreSQL, all async calls are indeed async all the way down, so there shouldn't be any sort of TP starvation issue as with SqlClient in dotnet/efcore#21818 - that may be why PG works OK (but the periodically slow SQL Server sounds like something to investigate as well). |
@martinmine Can you share how you managed to override them? I'm using IdentityServer4 (3.1.2) as a nuget package, so I do not see a way to do that. |
So where do we stand here? I think there's no change to be made in IdentityServer, correct? |
Closing. If you still have issues, feel free to reopen. |
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Comparing the performance on IdentityServer4 2.1.1 and 3.1.4 towards Microsoft SQL Server I have noticed that the query performance is significantly worse. With 2.1.1 we get 1.217ms avg. query time. With 3.1.4 the query time is up in 153ms. We can therefore not use this version in production. Looking at the logs, I can see that it runs a large set of queries torwards the Clients table:
After looking at the ClientStore class (https://github.com/IdentityServer/IdentityServer4/blob/3.1.4/src/EntityFramework.Storage/src/Stores/ClientStore.cs), I noticed that it is split up into many queries. By reverting the change added by c2dd330 (raised by #3718), I managed to a comparable performance benchmarks I got from 2.1.1 with 2.383ms avg. query time (the additional query time is however compensated with faster request/response time, where the new version with the custom
FindClientByIdAsync
has an average request time on 24ms, down from 41ms). I ended up with the following implementation ofFindClientByIdAsync
:The text was updated successfully, but these errors were encountered: