simpleidserver / SimpleIdServer

OpenID, OAuth 2.0, SCIM2.0, UMA2.0, FAPI, CIBA & OPENBANKING Framework for ASP.NET Core
https://simpleidserver.com/
Apache License 2.0
693 stars 91 forks source link

feat(SCIM): optimize performance #568

Closed danflomin closed 11 months ago

danflomin commented 11 months ago

In this PR the main changes relate to using async overloads of DB queries. Using async overloads is considered as the good practice in asp.net, and after a benchmark I conducted, these changes improved the performance by several magnitudes.

Also, the usage of yield in DB queries is considered to be less efficient with respect to network.

Another performance improvements introduced in here:

simpleidserver commented 11 months ago

Hello,

Firstly, thank you for your pull request.

Are you sure that the yield clause is less efficient in DB queries? I checked with and without the yield keyword, and the number of RPC requests made against the SQLSERVER is the same. It should also be the same with a POSTGRESQL database. In the pull request, all the data is stored in-memory, which can be problematic if there are many references to update.

I'm okay with using .NET version 7, but please remember to upgrade the other projects that reference this project.

Unfortunately, the solution is not compiling. Could you please fix the issues?

Thank you.

danflomin commented 11 months ago

Hello @simpleidserver

  1. Yes - I'm sure it is less efficient.
  2. Eventually we will have all the references in the handlers anyway, right?
  3. The change I made in the ResolveChildren and ResolveParents actually solved an OOM exception I previously got.
  4. I updated the PR and the build now works.

Kind regards Dan

simpleidserver commented 11 months ago

Unfortunately, the unit tests are not functioning properly :( Could you please revert your changes to the TargetFramework or modify the unit test projects accordingly?

Don't forget to include the changes made on the master branch.

Additionally, for my curiousity :), do-you have a link to documentation that explains why the yield keyword should not be used during database querying. As far as my understanding goes, when the yield statement is employed, the compiler transforms it into a state machine that implements IEnumerable. Subsequently, the foreach loop invokes the MoveNext method to retrieve results.

I have checked the performance, and there is an improvement in memory usage!

Ty again for your contribution.

simpleidserver commented 11 months ago

Hello @danflomin ,

I fixed the UTs and also issues in the RepresentationReferenceSync class. If you are agree with the changes, i'll merge them into the branch.

KR,

SID

danflomin commented 11 months ago

Hello

I will try to find a good explanation for the issue with the yield.

Thank you for your fixes, I went over most of it and it looks fine. I still need to understand the changes in RepresentationReferenceSync and in RepresentationSyncResult.

I will let you know once done.

Kind regards Dan

gabrielemilan commented 11 months ago

Hello, do you have ETA of this PR?

Kind regards. Gabriele