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

[Perf] Access tokens are listed too many times in AcquireTokenSilent / AcquireTokenForClient #2204

Closed
pmaytak opened this issue Nov 18, 2020 · 1 comment

Comments

@pmaytak
Copy link
Contributor

pmaytak commented Nov 18, 2020

This issue impacts scenarios of large token cache, which is common for client credential flows (300k+ tokens). MSAL copies access tokens from memory into a list several times, which is a perf impact (TBD - numbers)

Technical details:

InMemoryTokenCacheAccessor.GetAllAccessTokens() iterates through the whole AT collection and returns a List. When TokenCacheNotificationArgs.HasTokens property is set, it calls TokenCache.ITokenCacheInternal.HasTokensNoLocks() which eventually calls InMemoryTokenCacheAccessor.GetAllAccessTokens(). CacheSessionManager.RefreshCacheForReadOperationsAsync sets that property twice.

So the AT dictionary ends up being iterated 3 times for a read, 2 times for a save. This can have a perf hit when cache has a large number of tokens.

Possible enhancement: Update GetAllAccessTokens() to not create a list or reduce the number of calls to GetAllAccessTokens().

public IEnumerable<MsalAccessTokenCacheItem> GetAllAccessTokens()
{
// perf: do not call ConcurrentDictionary.Values as it takes a lock
List<MsalAccessTokenCacheItem> ats = new List<MsalAccessTokenCacheItem>();
foreach (var kvp in _accessTokenCacheDictionary)
{
ats.Add(kvp.Value);
}
return ats;
}

bool ITokenCacheInternal.HasTokensNoLocks()
{
return _accessor.GetAllRefreshTokens().Any() ||
_accessor.GetAllAccessTokens().Any(at => !IsAtExpired(at));
}

try
{
var args = new TokenCacheNotificationArgs(
TokenCacheInternal,
_requestParams.ClientId,
_requestParams.Account,
hasStateChanged: false,
TokenCacheInternal.IsApplicationCache,
hasTokens: TokenCacheInternal.HasTokensNoLocks(),
suggestedCacheKey: key);
await TokenCacheInternal.OnBeforeAccessAsync(args).ConfigureAwait(false);
}
finally
{
var args = new TokenCacheNotificationArgs(
TokenCacheInternal,
_requestParams.ClientId,
_requestParams.Account,
hasStateChanged: false,
TokenCacheInternal.IsApplicationCache,
hasTokens: TokenCacheInternal.HasTokensNoLocks(),
suggestedCacheKey: key);
await TokenCacheInternal.OnAfterAccessAsync(args).ConfigureAwait(false);
}

@pmaytak pmaytak self-assigned this Nov 18, 2020
@bgavrilMS bgavrilMS added bug and removed enhancement labels Nov 18, 2020
@bgavrilMS bgavrilMS changed the title [Feature Request] Improve calling GetAllAccessTokens method in internal cache [Perf] Access tokens are listed too many times in AcquireTokenSilent / AcquireTokenForClient Nov 18, 2020
@bgavrilMS bgavrilMS added this to the 4.24.0 milestone Nov 19, 2020
@neha-bhargava
Copy link
Contributor

Fixed in Msal release 4.24.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants