Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse lists in token cache filtering logic. #3233

Merged
merged 4 commits into from Mar 24, 2022
Merged

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Mar 17, 2022

Fixes #3178

Changes proposed in this request

  • Replace IReadOnlyList with List in accessor and filtering methods (RemoveAll is in concrete List implementation).
  • In FilterWithLogging, replace list = list.Where(predicate).ToList(); with list.RemoveAll(e => !predicate(e));
    • Add a boolean parameter whether to filter on the current list or create a new one.
  • In other methods, replace Where with RemoveAll and reverse filtering predicate (since instead of keeping the items based on a filter, we now remove items based on the opposite of that filter).

Testing
Existing tests.

Performance impact
Tested with 1 tenant and 10K tokens in cache (tenants don't matter in this scenario). About 40% reduction in memory allocated for client, OBO, and silent calls. Perf improvement is minimal, only about .5 ms for silent call.
Before

Method CacheSize Mean Error StdDev Median Min Max Gen 0 Allocated
AcquireTokenForClient (1, 10000) 5,353.3 μs 411.87 μs 1,207.93 μs 5,424.8 μs 2,887.60 μs 7,847.5 μs - 1,988 KB
AcquireTokenForOBO (1, 10000) 6,193.0 μs 341.06 μs 978.57 μs 6,045.6 μs 4,182.15 μs 8,638.4 μs - 2,256 KB
AcquireTokenSilent (1, 10000) 5,652.5 μs 378.56 μs 1,086.15 μs 5,605.9 μs 3,390.45 μs 8,480.9 μs - 2,249 KB
GetAccount (1, 10000) 184.6 μs 31.33 μs 87.85 μs 152.3 μs 92.75 μs 432.4 μs - 11 KB
GetAccounts (1, 10000) 175.9 μs 29.71 μs 81.82 μs 151.8 μs 95.20 μs 431.2 μs - 11 KB
RemoveAccount (1, 10000) 10,414.1 μs 211.16 μs 615.97 μs 10,262.3 μs 8,266.30 μs 12,159.5 μs 1000.0000 11,075 KB

After

Method CacheSize Mean Error StdDev Median Min Max Gen 0 Allocated
AcquireTokenForClient (1, 10000) 4,716.3 μs 349.99 μs 1,020.94 μs 4,567.2 μs 2,504.50 μs 6,964.0 μs - 1,219 KB
AcquireTokenForOBO (1, 10000) 5,997.8 μs 300.82 μs 882.25 μs 5,898.9 μs 4,146.40 μs 8,369.4 μs - 1,230 KB
AcquireTokenSilent (1, 10000) 4,847.9 μs 298.60 μs 861.54 μs 4,939.9 μs 2,915.85 μs 7,107.8 μs - 1,224 KB
GetAccount (1, 10000) 167.0 μs 29.82 μs 83.61 μs 129.5 μs 81.10 μs 449.9 μs - 10 KB
GetAccounts (1, 10000) 180.4 μs 36.04 μs 102.24 μs 138.6 μs 82.80 μs 514.6 μs - 10 KB
RemoveAccount (1, 10000) 10,475.1 μs 376.17 μs 1,085.33 μs 10,465.9 μs 7,783.30 μs 13,363.2 μs 1000.0000 10,562 KB

@pmaytak pmaytak marked this pull request as ready for review March 19, 2022 04:27
@pmaytak pmaytak force-pushed the pmaytak/perf-linq branch 2 times, most recently from 649e70e to c46c670 Compare March 19, 2022 06:04
@bgavrilMS
Copy link
Member

@pmaytak - did you invert the 2 perf tables in the description? Because the "before" looks better than the "After".

Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comment around filtering by tenantId first and then by clientid.

@pmaytak
Copy link
Contributor Author

pmaytak commented Mar 22, 2022

@pmaytak - did you invert the 2 perf tables in the description? Because the "before" looks better than the "After".

Yes, :) Corrected.

@pmaytak pmaytak requested a review from bgavrilMS March 23, 2022 05:47
@pmaytak pmaytak merged commit 7cc845d into master Mar 24, 2022
@pmaytak pmaytak deleted the pmaytak/perf-linq branch March 24, 2022 07:44
Csaba8472 added a commit to Csaba8472/microsoft-authentication-library-for-dotnet that referenced this pull request Apr 24, 2022
…library-for-dotnet

* 'master' of github.com:AzureAD/microsoft-authentication-library-for-dotnet: (39 commits)
  OBO for SP with RT support (AzureAD#3266) (AzureAD#3280)
  Update some projects to net48 (AzureAD#3269)
  Bump Source Link version (tool change) (AzureAD#3275)
  Update changelog.txt (AzureAD#3282)
  Default to WebView1 instead of WebView2 when using AAD or ADFS authorities (AzureAD#3276)
  Revert "OBO for SP with RT support (AzureAD#3266)"
  OBO for SP with RT support (AzureAD#3266)
  Add perf tests for Acquire Token calls with serialization cache and Builders (AzureAD#3250)
  Minor fix to SuggestedCacheKey comment and update NuGet icon in Readme. (AzureAD#3268)
  MSAL changelog 4.43.0 (AzureAD#3263)
  Bogavril/3251 (AzureAD#3255)
  Fix for AzureAD#3248 - use the correct plugin to sing out (AzureAD#3253)
  Performance project improvements (AzureAD#3064)
  Reuse lists in token cache filtering logic. (AzureAD#3233)
  Update andorid install script to use andorid 30 (AzureAD#3243)
  Fix for UWP packaging (AzureAD#3239)
  Pass account to Auth result (AzureAD#3199)
  App ported to Lab4 (AzureAD#3229)
  Trwalke/ruleset updates (AzureAD#3189)
  Conditional Access for Android (AzureAD#3210)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Improve token filtering logic
3 participants