[Prototype] Add cache size limit and eviction. by pmaytak · Pull Request #3104 · AzureAD/microsoft-authentication-library-for-dotnet

pmaytak · 2022-01-12T08:27:06Z

Fixes #3020

Changes proposed in this request

Added size limit option to the CacheOptions public API, which represent cache size in bytes.
Added cache capacity properties to the user and app cache structures and a property that tracks combined cache size to a parent level TokenCache class.
These properties are updated during add and remove operations. Constant token size is used. -
The capacity overflow check is done when adding access tokens.
If capacity is reached, cache is cleared, app or user, depending on which operation triggered it.

Issues:

It is difficult to keep the current cache size in sync in multi-threaded environments. When clearing the cache, after it hits capacity, a lock is needed to make sure the current size counter is not updated by other cache methods, since it’s set to 0. Locks can cause performance issues, as in past cases. Current cache size tracker also cannot be calculated dynamically based on the count of dictionary elements, since the storage structure is not a simple dictionary, but a nested one.
With the current cache structure, there are app and user cache accessors, which are separate from each other. So, a cache size tracker is needed in a class above them, which increases the complexity of keeping track of the three capacity variables.

Testing
To be added.

Performance impact
Should be no impact - in add/remove accessor methods, there're a few O(1) dictionary calls.

SameerK-MSFT · 2022-01-12T17:13:59Z

+        /// </summary>
+        /// <param name="useSharedCache">Set to true to share the cache between all ClientApplication objects. The cache becomes static. <see cref="UseSharedCache"/> for a detailed description. </param>
+        /// <param name="sizeLimit">Token cache size limit in bytes. <see cref="SizeLimit"/> for a detailed description.</param>
+        public CacheOptions(bool useSharedCache, long sizeLimit)


Can we have default values here (as per any recommended practices)?

I don't think we can provide a realistic default value; it would be based on the user's scenario.

Yeah, that's a good question we're struggling with :). What would be a good default? ...

If default varies widely based on the user scenarios, we cannot do much. But if it primarly leans towards a certain value, that can be used. But we do not have answer, then it is good as it is.

Danny also touched on a point of should there bit a minimum. Currently if someone specifies small value (like 1), cache will be compacted on each operation.

SameerK-MSFT · 2022-01-12T17:22:47Z

@@ -216,13 +234,25 @@ public void SetiOSKeychainSecurityGroup(string keychainSecurityGroup)
        public virtual void Clear()
        {
            AccessTokenCacheDictionary.Clear();


Threading issue: Between Clear and setting CacheSize to zero, something may get added to the dictionary and the two can go out of sync

True, but we also don't lock on cache operations in general (based on eventual consistency principle). So this behavior can happen with other add/remove methods.

Eventual consistency is not guaranteed here. For example:

Thread 1 calls the AccessTokenCacheDictionary.Clear() method and is paused.

Thread 2 adds an item and updates the _appCacheSize.

Thread 1 resumes execution and sets the appCacheSize to 0 (Interlocked.Exchange(ref _appCacheSize, 0)).

Now there is one item in the cache but the size is 0.

The same issue applies to the TokenCache.CacheSize, and in this case the error can accumulate over multiple Clear() calls.

@dannybtsai Do you suggest locking? Only when clearing or always when updating the cache size?

It is hard to achieve consistency without locking as far as I can tell. And you need to always lock, not just when clearing cache otherwise it is the same problem.

…ion-3020

pmaytak · 2022-02-11T08:40:45Z

+        /// IMPORTANT: Monitor app health metrics (including memory usage) and cache performance (<see href="https://aka.ms/msal-net-token-cache-serialization"/>)
+        /// and adjust size limit accordingly.
+        /// </remarks>
+        public long? SizeLimit


Size limit applies only when cache serialization is disabled.

pmaytak · 2022-02-11T08:43:27Z

+        private void Compact()
+        {
+            _logger.Always("[UserCache] Compacting cache.");
+            Clear();


Clearing the cache because it's simpler and other forms of compaction become complex for the user cache. (For ex. removing only ATs will leave the user cache with non-AT tokens; trying to remove all tokens for a user doesn't work for OBO since partition keys are different; randomly removing token entries will leave orphaned/unassociated tokens.)

pmaytak · 2022-02-11T08:45:25Z

+        /// <summary>
+        /// Static, used by both app and user caches to track approximate token cache size.
+        /// </summary>
+        internal static long CacheSize = 0L;


Cache size includes user+ app token cache. 2 instances of TokenCache class are created (1 with app cache accessors, 1 with user accessors). So moved this size property up a level so it can be accessed by both. However, because of this design, clearing the cache is only done for that respective cache, app or user.

bgavrilMS · 2022-02-11T11:16:20Z

        private readonly CacheOptions _tokenCacheAccessorOptions;

+        // Approximate size of cache item objects
+        private const long AccessTokenSizeInBytes = 4500;


My main concern is that a constant approximation will not work well:

AAD allows for configuration of optional claims

POP tokens are a bit bigger because they contain the POP key id

AAD is adding support for SAML tokens (there is already an OBO flow for this). We won't be able to update this aproximation for all token types.

I think there are a few ways out of this:

Allow customers to define this constant (this is what SAL does). But this is a pretty "obscure" scenario

Use non-constant size approximation, i.e. we know the size of the actual token (it's a string) at which we add some metadata (fairly constant)

Do not expose sizeLimit, expose countLimit

@dannybtsai @henrik-me - can you please review this strategy?

Is there a reason to use a constant value instead of the actual size (the #2 option)?

@bgavrilMS

Hmm, interesting, I'd like to see the implementation in SAL. How would customers know that constant?

Yea, metadata should be fairly constant. This does seem like a better approximation.

Specific to the user cache, what would the count represent: only access tokens, all tokens? Because in both cases, the count would not represent the whole cache size accurately.

@dannybtsai
How would you measure the actual size of objects in memory? We could measure the strings size in cache objects like Bogdan mentioned below and in Microsoft.Identity.Web we serialize the whole cache, so take size that way. But in this case cache entry is an object.

dannybtsai · 2022-02-11T19:46:36Z

internal class InMemoryPartitionedAppTokenCacheAccessor : ITokenCacheAccessor

Why Compact() clears the entire cache instead of reducing the cache by a certain percentage and still keeping some? #Closed

Refers to: src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs:23 in 64f8718. [](commit_id = 64f8718, deletion_comment = False)

pmaytak · 2022-02-12T06:41:11Z

internal class InMemoryPartitionedAppTokenCacheAccessor : ITokenCacheAccessor
Why Compact() clears the entire cache instead of reducing the cache by a certain percentage and still keeping some? #Closed

Refers to: src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedAppTokenCacheAccessor.cs:23 in 64f8718. [](commit_id = 64f8718, deletion_comment = False)

Basically clearing full cache is simpler. With the user cache, there are multiple token types saves in different dictionaries, so clearing them in a sensible manner becomes complex. I documented this in a doc, I'll share with the team.

dannybtsai · 2022-02-15T21:39:28Z

+            // Update cache size only if cache item is added, not updated
+            if (!AccessTokenCacheDictionary.TryGetValue(partitionKey, out var partition) || !partition.TryGetValue(itemKey, out _))
+            {
+                Interlocked.Add(ref _appCacheSize, AccessTokenSizeInBytes);


_appCacheSize

Is it really necessary to use a variable to track the size? Isn't _appCacheSize == AccessTokenCacheDictionary.Count * AccessTokenSizeInBytes?

There seems to be some risk of not setting the size correctly even though Interlocked methods are atomic. And it is more complicated to set them in the Clear() method without locking.

ConcurrentDictionary<string, ConcurrentDictionary<string, MsalAccessTokenCacheItem>> AccessTokenCacheDictionary
TokenCacheDictionaries are dictionary of dictionaries. with the outer key being a partition key and an inner key is an actual cache item key. So unfortunately count does not work.

If AccessTokenSizeInBytes is added to the _appCacheSize every time a token is saved, isn't _appCacheSize == AccessTokenCacheDictionary.Count * AccessTokenSizeInBytes?

dannybtsai · 2022-02-15T21:57:59Z

+            if (!AccessTokenCacheDictionary.TryGetValue(partitionKey, out var partition) || !partition.TryGetValue(itemKey, out _))
+            {
+                Interlocked.Add(ref _appCacheSize, AccessTokenSizeInBytes);
+                Interlocked.Add(ref TokenCache.CacheSize, AccessTokenSizeInBytes);


TokenCache.CacheSize

Can this be calculated?

Hmm, I don't think so. There are two instances of TokenCache class created and then one instance has app token cache accessor and one has user accessor, but they don't know about each other. And I don't think CacheSize can be moved into a higher parent class.

bgavrilMS · 2022-04-15T12:19:10Z

Can we close this one @pmaytak ?

Initial. Add app cache size limit, check, and cache clearing.

caf1827

SameerK-MSFT suggested changes Jan 12, 2022

View reviewed changes

pmaytak added 2 commits February 10, 2022 21:00

Merge remote-tracking branch 'origin/master' into pmaytak/cache-evict…

5f4559d

…ion-3020

Add cache limit and eviction functionality.

64f8718

pmaytak changed the title ~~Initial. Add app cache size limit, check, and cache clearing.~~ Add cache size limit and eviction. Feb 11, 2022

pmaytak marked this pull request as ready for review February 11, 2022 08:28

pmaytak requested review from SameerK-MSFT, bgavrilMS, gladjohn, neha-bhargava and trwalke February 11, 2022 08:29

pmaytak commented Feb 11, 2022

View reviewed changes

Comment thread ...icrosoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs

bgavrilMS reviewed Feb 11, 2022

View reviewed changes

Comment thread src/client/Microsoft.Identity.Client/AppConfig/CacheOptions.cs Outdated

bgavrilMS reviewed Feb 11, 2022

View reviewed changes

Comment thread src/client/Microsoft.Identity.Client/AppConfig/CacheOptions.cs

bgavrilMS reviewed Feb 11, 2022

View reviewed changes

dannybtsai reviewed Feb 11, 2022

View reviewed changes

Comment thread src/client/Microsoft.Identity.Client/AppConfig/CacheOptions.cs Outdated

dannybtsai reviewed Feb 11, 2022

View reviewed changes

Comment thread src/client/Microsoft.Identity.Client/AppConfig/CacheOptions.cs

dannybtsai reviewed Feb 15, 2022

View reviewed changes

Address feedback.

ccb06a5

pmaytak marked this pull request as draft February 23, 2022 07:23

pmaytak changed the title ~~Add cache size limit and eviction.~~ [Prototype] Add cache size limit and eviction. Feb 23, 2022

pmaytak closed this Apr 15, 2022

pmaytak mentioned this pull request Jul 6, 2022

[Prototype] Use IIdentityCache and CompositeCache #3439

Closed

bgavrilMS deleted the pmaytak/cache-eviction-3020 branch August 11, 2022 17:43

Conversation

pmaytak commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmaytak Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannybtsai Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dannybtsai commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmaytak commented Feb 12, 2022

Uh oh!

dannybtsai Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS commented Apr 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pmaytak commented Jan 12, 2022 •

edited

Loading

pmaytak Feb 11, 2022 •

edited

Loading

dannybtsai Feb 11, 2022 •

edited

Loading

dannybtsai commented Feb 11, 2022 •

edited

Loading

dannybtsai Feb 15, 2022 •

edited

Loading