Fix eager evaluation in ConcurrentDictionary.GetOrAdd calls and add r…#5950
Merged
Fix eager evaluation in ConcurrentDictionary.GetOrAdd calls and add r…#5950
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Improves performance by preventing unnecessary object allocation in ConcurrentDictionary.GetOrAdd calls and adds a repo-wide Copilot review rule to avoid reintroducing eager GetOrAdd(key, value) patterns.
Changes:
- Switched
GetOrAdd(partitionKey, value)toGetOrAdd(partitionKey, valueFactory)in the in-memory user token cache to avoid eager allocations on cache hits. - Added a Copilot code review rule documenting the
ConcurrentDictionary.GetOrAddeager-evaluation pitfall and the preferred factory-delegate pattern.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/InMemoryPartitionedUserTokenCacheAccessor.cs |
Uses the GetOrAdd factory overload to avoid allocating inner dictionaries on every cache hit when saving refresh tokens, ID tokens, and accounts. |
.github/copilot-instructions.md |
Adds a review rule explaining why GetOrAdd(key, valueFactory) should be preferred to prevent eager allocations. |
bgavrilMS
commented
Apr 24, 2026
gladjohn
approved these changes
Apr 24, 2026
trwalke
approved these changes
Apr 29, 2026
neha-bhargava
approved these changes
Apr 29, 2026
…eview rule GetOrAdd(key, value) eagerly evaluates the second argument before checking the dictionary. When the value is a constructor call, a new object is created and immediately discarded on every cache hit. Fixed in InMemoryPartitionedUserTokenCacheAccessor: unnecessary ConcurrentDictionary allocated on every SaveRefreshToken, SaveIdToken, and SaveAccount call. Also added a code review rule to copilot-instructions.md to prevent this pattern from being reintroduced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bgavrilMS
force-pushed
the
bogavril/fix-getorAdd-eager-evaluation
branch
from
3a7ff6a to
939502b
Compare
gladjohn
approved these changes
Apr 30, 2026
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
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.
…eview rule
GetOrAdd(key, value) eagerly evaluates the second argument before checking the dictionary. When the value is a constructor call, a new object is created and immediately discarded on every cache hit.
Fixed in InMemoryPartitionedUserTokenCacheAccessor: unnecessary ConcurrentDictionary allocated on every SaveRefreshToken, SaveIdToken, and SaveAccount call.
Also added a code review rule to copilot-instructions.md to prevent this pattern from being reintroduced.
Fixes #
Changes proposed in this request
Testing
Performance impact
Documentation