feat(SCIM): optimize performance #568

danflomin · 2023-08-23T08:38:34Z

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:

The ResolveChildren and ResolveParents functions, queried the same representations multiple times, so I added a check that makes sure that each representation is queried only once.
The implementation of GET methods can use the AsNoTracking behavior which improves performance.
I used the paginated variation in a few places that did not use it.

danflomin · 2023-08-23T08:39:59Z

src/Scim/SimpleIdServer.Scim.Persistence.EF/Extensions/EFSCIMExpressionLinqExtensions.cs

                result.FlatAttributes = filteredAttrs.ToList();
                return result;
            }
-
-            representations = representations.Include(r => r.FlatAttributes).ThenInclude(s => s.SchemaAttribute);


This was moved to EFSCIMRepresentationQueryRepository

danflomin · 2023-08-23T08:46:40Z

src/Scim/SimpleIdServer.Scim/Helpers/RepresentationReferenceSync.cs

 		}

-		public virtual IEnumerable<RepresentationSyncResult> Sync(string resourceType, SCIMRepresentation oldScimRepresentation, SCIMRepresentation newSourceScimRepresentation, string location, SCIMSchema schema, bool updateAllReferences = false, bool isScimRepresentationRemoved = false)
+        public virtual async Task<List<RepresentationSyncResult>> Sync(string resourceType, SCIMRepresentation oldScimRepresentation, SCIMRepresentation newSourceScimRepresentation, string location, SCIMSchema schema, bool updateAllReferences = true, bool isScimRepresentationRemoved = false)


I rebased my code which originated from 4.0.2 using master, and I see that you made some changes like false -> 'true.
@thabart should I change this?

I checked the master branch, by default, the field updateAllReferences is equals to false and not to true :

SimpleIdServer/src/Scim/SimpleIdServer.Scim/Helpers/RepresentationReferenceSync.cs

Line 18 in ed07717

public class RepresentationReferenceSync : IRepresentationReferenceSync

The field must be false

danflomin · 2023-08-23T08:47:09Z

src/Scim/SimpleIdServer.Scim/Helpers/RepresentationReferenceSync.cs

                }

                foreach (var attributeMapping in kvp.Where(r => !r.IsSelf))
                {
                    result = new RepresentationSyncResult();
-                    var targetSchema = mappedSchemas.First(s => s.ResourceType == attributeMapping.TargetResourceType && s.Attributes.Any(a => a.Id == attributeMapping.TargetAttributeId));
+                    var targetSchema = targetSchemas.First(s => s.ResourceType == attributeMapping.TargetResourceType);


same about this one

"This portion of code has been updated by pull request #557.
There was an issue during the resolution of the attribute present in a custom schema.
You are welcome to retain this change :)

danflomin · 2023-08-23T08:47:33Z

src/Scim/SimpleIdServer.Scim/Helpers/RepresentationReferenceSync.cs

                    var targetAttribute = targetSchema.GetAttributeById(attributeMapping.TargetAttributeId);
-                    var sourceAttribute = sourceSchema.GetAttributeById(attributeMapping.SourceAttributeId);
+                    var sourceAttribute = schema.GetAttributeById(attributeMapping.SourceAttributeId);


Same remark

simpleidserver · 2023-08-23T13:15:07Z

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 · 2023-08-23T14:04:41Z

Hello @simpleidserver

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

Kind regards
Dan

danflomin · 2023-08-23T08:49:44Z

src/Scim/SimpleIdServer.Scim/Helpers/RepresentationReferenceSync.cs

+                var targetSchema = targetSchemas.Single(s => s.Name == propagatedAttribute.TargetResourceType);
+                var sourceSchema = targetSchemas.Single(s => s.Name == propagatedAttribute.SourceResourceType);


simpleidserver · 2023-08-23T19:57:20Z

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.

Fix the UTs

…4-flomin-optimize

simpleidserver · 2023-08-24T18:55:57Z

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 · 2023-08-27T12:35:07Z

src/Scim/SimpleIdServer.Scim/SimpleIdServer.Scim.csproj

@@ -2,11 +2,13 @@
  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <Description>SCIM2.0 implementation.</Description>
+	<LangVersion>latest</LangVersion>


why is this needed?

Because some changes in the pull request are not supported by previous c# version for example ??=

danflomin · 2023-08-27T12:46:30Z

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 · 2023-08-28T16:52:36Z

Hello, do you have ETA of this PR?

Kind regards.
Gabriele

flomindanza added 5 commits August 23, 2023 11:22

optimize

eb11b90

small refactor

a7af764

optimize: add AsNoTracking for GET

e5ee65f

use List<> in DefaultAttributeMappingQueryRepository

eb94b05

bla

05e6498

danflomin commented Aug 23, 2023

View reviewed changes

danflomin force-pushed the release/4.0.4-flomin-optimize branch from a0da371 to d073474 Compare August 23, 2023 08:42

danflomin commented Aug 23, 2023

View reviewed changes

danflomin force-pushed the release/4.0.4-flomin-optimize branch 3 times, most recently from 2d821a1 to d8acd2f Compare August 23, 2023 08:50

CR fix

89fb08d

danflomin force-pushed the release/4.0.4-flomin-optimize branch from d8acd2f to 89fb08d Compare August 23, 2023 08:51

simpleidserver mentioned this pull request Aug 23, 2023

[SCIM] Error on patch group 4.0.3 #567

Closed

fix build and change to net7.0

af2b8be

danflomin commented Aug 23, 2023

View reviewed changes

danflomin force-pushed the release/4.0.4-flomin-optimize branch 2 times, most recently from 50bca19 to 7b0baf8 Compare August 24, 2023 09:04

align with main

8a4f81e

danflomin force-pushed the release/4.0.4-flomin-optimize branch from 7b0baf8 to 8a4f81e Compare August 24, 2023 13:07

thabart added 3 commits August 24, 2023 17:46

Use "netstandard2.0"

9a3fc97

Fix the UTs

Merge remote-tracking branch 'origin/release/4.0.4' into release/4.0.…

c6e101c

…4-flomin-optimize

Fix UTs

2346789

danflomin commented Aug 27, 2023

View reviewed changes

simpleidserver changed the base branch from master to release/4.0.4 August 29, 2023 09:49

simpleidserver merged commit b5d811f into release/4.0.4 Aug 29, 2023
1 check was pending

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(SCIM): optimize performance #568

feat(SCIM): optimize performance #568

danflomin commented Aug 23, 2023

danflomin Aug 23, 2023

danflomin Aug 23, 2023

simpleidserver Aug 23, 2023

danflomin Aug 23, 2023

simpleidserver Aug 23, 2023

danflomin Aug 23, 2023

simpleidserver Aug 23, 2023

simpleidserver commented Aug 23, 2023 •

edited

Loading

danflomin commented Aug 23, 2023

danflomin Aug 23, 2023

simpleidserver commented Aug 23, 2023 •

edited

Loading

simpleidserver commented Aug 24, 2023

danflomin Aug 27, 2023

simpleidserver Aug 27, 2023

danflomin commented Aug 27, 2023

gabrielemilan commented Aug 28, 2023

		var targetSchema = targetSchemas.Single(s => s.Name == propagatedAttribute.TargetResourceType);
		var sourceSchema = targetSchemas.Single(s => s.Name == propagatedAttribute.SourceResourceType);

feat(SCIM): optimize performance #568

feat(SCIM): optimize performance #568

Conversation

danflomin commented Aug 23, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simpleidserver commented Aug 23, 2023 • edited Loading

danflomin commented Aug 23, 2023

Choose a reason for hiding this comment

simpleidserver commented Aug 23, 2023 • edited Loading

simpleidserver commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danflomin commented Aug 27, 2023

gabrielemilan commented Aug 28, 2023

simpleidserver commented Aug 23, 2023 •

edited

Loading

simpleidserver commented Aug 23, 2023 •

edited

Loading