-
Notifications
You must be signed in to change notification settings - Fork 340
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
Perf improvements to AcquireTokenForClient flow #2261
Conversation
src/client/Microsoft.Identity.Client/Cache/CacheSessionManager.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Client.Performance/GetAllAccessTokensTests.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Client.Performance/GetAllAccessTokensTests.cs
Outdated
Show resolved
Hide resolved
Would really like to see numbers for .net 4.6.1 runtime to be able to compare with 4.8 runtime |
Wondering if we should add a hashmap to make lookup faster? |
tests/Microsoft.Identity.Test.Unit/CacheTests/TokenCacheNotificationTests.cs
Outdated
Show resolved
Hide resolved
… Add a check for existing cache delegates before calling them.
Using normal Where instead of FilterWithLogging. Add checks if the cache delegates were set, before calling them. Add performance test project. Update TokenCacheHelper to be able to add N tokens.
5c390b2
to
4e78505
Compare
// if only one cached token found | ||
if (filteredItems.Count() == 1) | ||
if (filteredItems.Count == 1) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is much better.
} | ||
|
||
return metadata; | ||
return _appMetadataDictionary.Select(kv => kv.Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to removing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
return rts; | ||
return _refreshTokenCacheDictionary.Select(kv => kv.Value).ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bgavrilMS As discussed, added ToList
in these methods, since we need to investigate what the impact of returning an IEnumerable
is.
Issue #2204
Changes include:
GetAllAccessTokens
method to returnIEnumerable
instead of aList
.IEnumerable.Where
instead ofFilterWithLogging
, inITokenCacheInternal.FindAccessTokenAsync
.Added a BenchmarkDotNet performance test project to test the
AcquireTokenForClient
. Tested with cache size of 100, 1k, 10k, and 100k items. Also ran the test multiple times. There was some spread in mean data points between the runs (although I'm not sure why). (The spread was wider pre-fixes and much narrower post-fixes, except for .NET 4.8 which basically had no spread.) .NET 5 seems to be slightly faster than .NET Core 3.1 which is slightly faster than .NET 4.8. Below is the summary data..NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203)
Using in-memory token serialization cache yielded very similar results post-fix.
With in-memory token serialization cache:
.NET Core 5.0.0 (CoreCLR 5.0.20.51904, CoreFX 5.0.20.51904)
.NET Framework 4.8 (4.8.4250.0 runtime)
.NET Framework 4.6.1 (4.8.4250.0 runtime)
Machine specs:
.NET Framework 4.6.1 (4.8.3928.0 runtime)
.NET Framework 4.6.1 (4.6.1055.0 runtime)
Machine specs: