Skip to content

[Prototype] L1 Wilson LRU cache#3177

Closed
pmaytak wants to merge 1 commit into
masterfrom
pmaytak/L1-cache
Closed

[Prototype] L1 Wilson LRU cache#3177
pmaytak wants to merge 1 commit into
masterfrom
pmaytak/L1-cache

Conversation

@pmaytak

@pmaytak pmaytak commented Feb 22, 2022

Copy link
Copy Markdown
Contributor

Fixes #

Changes proposed in this request

  • Added maximum items option to the CacheOptions public API, which represents the count of items in the cache.
  • Added an instance of Wilson cache, as an L1Cache, to each TokenCache instance with key being a partition string and value being a user or app accessor.
  • Added GetAllValues method to Wilson cache.
  • In the cache accessor classes, remove the partitions from the token dictionaries – the partitions are now at the TokenCache level, one level above accessors.
  • Replaced the Accessor property in the TokenCache class with GetAccessor method which retrieves the correct accessor instance from the Wilson cache.

Issues:

  • Wilson cache capacity represents a count of keys in the internal dictionary, and not size. This results in an inconsistent size measurement, since each partition can have a different number of tokens in it.
  • Presently, an instance of public/confidential client app contains a reference to two TokenCache instances (one for app cache and one for user cache) which in turn have an instance of a cache accessor. These cache and accessor instances are abstracted from each other. This creates a complexity of finding a good location for the Wilson cache. The prototype has two Wilson cache instances, for app and user caches.
  • Wilson cache doesn’t have a GetAll method. GetAll (for example, GetAllAccessTokens and similar) methods without a partition key are called primarily during serialization/deserialization in confidential client apps. Technically in this scenario, the cache would only have one partition, but still the call pattern would have to be complex, since a collection of accessors would be returned, and then iterated over to get all tokens for each. In the same vein, there’s GetAccounts method in public client apps, which would have to iterate through all partitions/accesors, adding to the complexity.
  • There are about 500 references to the Accessor property in the code, albeit only about 40 being from the main library and the rest from the test code. However, it still means that changing this code will lead to wide reaching changes throughout the code base.

Testing

Performance impact

…rty to TokenCache class. Update some methods that called Accessor property to call GetAccessor method.
@pmaytak pmaytak changed the title L1 Wilson LRU cache L1 Wilson LRU cache WIP Feb 22, 2022
@pmaytak pmaytak changed the title L1 Wilson LRU cache WIP [Prototype] L1 Wilson LRU cache Feb 23, 2022
return cacheItem != null;
}

public IList<TValue> GetAllValues()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dannybtsai Was there any talks about adding a GetAllValues method (kind of like this) that returns actual values and not just CacheEntries?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pmaytak that could be expensive as we have seen 100,000+ items in the cache.

{
internal delegate void ItemRemoved(TValue Value);

private readonly int _capacity;

@pmaytak pmaytak Feb 24, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dannybtsai Similarly, any discussions about allowing a user to specify current quantity themselves? For example, kind of like .NET MemoryCache allows to specify the cache entry size in a Set method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pmaytak is there a way we can use the code directly from M.IM.Tokens?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can add MSAL as a friend.

@bgavrilMS

Copy link
Copy Markdown
Member

Can we close this one @pmaytak ?

@pmaytak pmaytak closed this Apr 15, 2022
@bgavrilMS bgavrilMS deleted the pmaytak/L1-cache branch August 11, 2022 17:44
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.

3 participants