Reduce memory consumption by avoiding adding items when queue is full #3374
+112
−496
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Do not add items if cache is full or compacting.
- Remove TaskCount, this is internal, but is not used outside of the assembly so can be removed - Also remove task count from InMemoryCryptoProviderCache - Make fields readonly as appropriate - Fix a few comments - Remove tests that are no longer relavent
Contributor
|
@copilot Generate summary of the changes and separate the source and test changes. |
5 tasks
Contributor
Copilot finished reviewing on behalf of
pmaytak
December 2, 2025 17:18
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces memory consumption in the EventBasedLRUCache by preventing items from being added when the cache is full or compacting. Under heavy load, the cache was unable to keep up, so a change was made to reject new items when the cache is at or above 95% capacity.
Key changes include:
- Converting
SetValuetoTrySetValuethroughout the codebase to allow callers to detect when items are not added - Simplifying the task lifecycle by starting the event queue task at construction and running it for the lifetime of the cache
- Ensuring SignatureProviders are actually added to the cache before associating them with the cache instance
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| EventBasedLRUCache.cs | Main implementation changes: added early return when compacting, simplified task lifecycle from start/stop to long-running, added TrySetValue methods, improved consistency in map/linked list operations |
| InMemoryCryptoProviderCache.cs | Fixed spelling errors (Trys → Tries), added check to ensure TrySetValue succeeds before associating provider with cache, updated Dispose to use StopEventQueueTaskImmediately |
| EventBasedLRUCacheTests.cs | Updated all SetValue calls to TrySetValue, added tests for new behavior where items are not added when cache is full, added Collection attribute for test isolation |
| CryptoProviderFactoryTests.cs | Removed obsolete tests related to task lifecycle management that are no longer applicable with the new long-running task design |
| BaseConfigurationManager.cs | Updated to use TrySetValue instead of SetValue for consistency |
| InternalAPI.Shipped.txt | Removed TaskCount property that is no longer needed |
| InternalAPI.Unshipped.txt | Added new TrySetValue methods and StopEventQueueTaskImmediately to public API surface |
test/Microsoft.IdentityModel.Tokens.Tests/CryptoProviderFactoryTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #3239
Do not add items if cache is full or compacting.
Under heavy load, stress tests showed that the LRUCache was unable to keep up a change was made to avoid adding to the cache if we detect it was full or compacting.
A change to start TaskEventQ on startup instead of starting and stopping improves performance and simplifies the code.
We were not checking is a SignatureProvider was actually added to the cache. This is important as if the SignatureProvider is not added, the cache will not be able to properly apply the dispose logic and kernel objects could pile up.