[Bug] Perf degradation of AcquireTokenForClient due to default partitioned cache

[pmaytak]

Is your feature request related to a problem? Please describe.
Starting in MSAL 4.30, there's a default in-memory partitioned cache for confidential client applications. For each cache operation, the data is serialized/deserialiazed, which causes a performance hit. Seems to be a bigger issue for apps that have single-tenant partitions with many resources per tenant.

Possible solutions

1. Partition the internal cache.

• Create PartitionedInMemoryTokenCacheAccessor that implements ITokenCacheAccessor and is similar to InMemoryTokenCacheAccessor except that the token dictionaries are partitioned by tenant id.
• Add overloaded GetAllX methods that accept a tenant parameter.
• In TokenCache, for confidential client app, set the accessor to the partitioned one.

2. Explore having a smaller partition key; currently it's client ID + tenant ID.

• In SuggestedWebCacheKeyFactory, add requestParams.Scope to the app cache key string.

3. When searching through the internal cache, find token by key first then go through filters.

• In TokenCache.ITokenCacheInternal.FindAccessTokenAsync, call _accessor.GetAccessToken and pass a created MsalAccessTokenCacheKey. Some [additional filtering] might still need to be done.

Also add performance tests to compare before and after change. (Testing scenario should include single- and multi-tenant cases with many resources.)

[bgavrilMS]

Let's treat this as a performance regression. Since before MSAL 4.30, AcquireTokenForClient would not use any JSON operation, but afterwards it does, leading to increased number of allocations.

[bgavrilMS]

Have a look at ITokenCacheAccessor - maybe we can have a smarter implementation there for app token cache only.

[rymeskar]

+1, had a similar discussion with @bgavrilMS and @jennyf19 yesterday on the amount of tokens in a single cache line in CCA when targeting multiple downstream dependencies.

One of the quick-non-breaking options discussed for cache implementors is to provide some more context in the TokenCacheNotificationArgs (especially the scopes and whatever else is used for in-cache row filtering) so that the Suggested Cache Key can be extended and thus creating more granular partitions. For example a second property Suggested Fully Token Partitioned Key. This still has the downside of working with json serialization :( Just for a single token.

[sea1jxr]

Ideally there would be a way to skip all serialization for an in memory cache.

[bgavrilMS]

@pmaytak - I think you should focus on idea 1, i.e. a token cache accessor that is key based, i.e. pre-filtered by tenant id. If this works, we can think about generalizing this idea, maybe the app developer can tell us "partition by tenant id AND scope".

For idea 2 - we cannot change the SuggestedCacheKey as it would break existing token caches. Instead, we can expose more fields in TokenCacheNotificationArgs as @rymeskar suggests.

For perf testing, you can use the WebAPI project to simulate high load or the Benchmarking approach. I think the WebAPI looks at things more holistically and is easier to setup up.

[jennyf19]

I thought we were doing Perf testing consistently and on PRs impacting the cache, so why did we miss this perf issue? are we missing tests? wasn't the cache serialized/deserialized before 4.30 release? Do you we have current numbers on perf compared w/previous versions, to see how much of a perf issue we are talking about.

[pmaytak]

Included in MSAL.NET 4.36.0 release.

[rymeskar]

It's great to see the rate at which you manage to improve the caching and resiliency experience in the past weeks and months.

At this front, @pmaytak and @bgavrilMS I was wondering whether are you planning to add support for more granular partitioning even for the non-in-memory-cache scenarios?

[jmprieur]

@rymeskar this is already realized thanks to the SuggestedCacheKey property of the TokenSerializationArgs.

[rymeskar]

My understanding was that SuggestedCacheKey is coarsely partitioned; i.e. it does not partition by scope/resourceId.

[jmprieur]

Being partitioned by resource ID would not allow for using the refresh token to acquire a token for a different resource.

[rymeskar]

This makes sense.
For CCA and for app-only scenarios, this should be fine. For CCA and OBO, should we give the customers an otpion to pick either better RT re-use or finer granularization? After all the RT is re-usable just for the validity period of the original user assertion.

[jmprieur]

nom @rymeskar the RT for OBO token is reusable for 90 days (provided, today you give the userassertion, which might have expired, but is used as a key to the cache). @pmaytak is actually improving the experience for OBO tokens used in long running processes:
#2820