Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order scopes on save, and optimize the happy path for access token read #644

Merged
merged 3 commits into from Jan 9, 2024

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Jan 3, 2024

This PR currently contains 2 commits.

  1. The first commit orders scopes on save, which is similar to Order scopes on save to avoid access token duplication in the cache microsoft-authentication-library-for-dotnet#4479. Existing unit tests need adjustment to match the changed scope order.
  2. The second commit optimizes the happy path of access token read, which was the idea mentioned in this conversation. Basically, when searching for an access token, this PR uses the token key to attempt an O(1) search, and falls back to O(n) search when necessary.
    • No other unit test adjustment is needed, meaning this change is backward compatible.
    • Existing benchmark test shows ~40% time reduction (or 1.6x speed improvement) on "token cache hit" code path, which is also visible in diagram No. 2 and No. 4 in this benchmark page. (Note that the performance gain can be much higher than 1.6x if each tenant has more tokens; it is actually an O(1) vs O(n) improvement.)

@rayluo rayluo requested a review from bgavrilMS January 3, 2024 09:57
@bgavrilMS
Copy link
Member

Nice perf result. CC @pmaytak as the same strategy could be done in .NET

@rayluo rayluo merged commit 804d529 into dev Jan 9, 2024
12 checks passed
@rayluo rayluo deleted the order-scopes branch January 9, 2024 00:44
@rayluo rayluo mentioned this pull request Feb 22, 2024
now = time.time()
refresh_reason = msal.telemetry.AT_ABSENT
for entry in matches:
for entry in self.token_cache._find( # It returns a generator
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing token_cache.find to token_cache._find makes this line no longer call the find method on msal.token_cache.TokenCache's subclasses, e.g. msal_extensions.token_cache.PersistedTokenCache.find, but directly msal.token_cache.TokenCache._find.

This causes all logic in msal_extensions.token_cache.PersistedTokenCache.find being skipped:

https://github.com/AzureAD/microsoft-authentication-extensions-for-python/blob/a10d0929b53d5397a3d5883e97b6925de0252e2b/msal_extensions/token_cache.py#L72-L87

This method contains 2 important functionalities:

  1. Load token cache from a file
  2. Handle concurrent situation

When a PersistedTokenCache is provided to ConfidentialClientApplication, as token cache is no longer loaded from a file, self._cache becomes an empty dict when this line is called:

return self._cache.get(credential_type, {}).get(key, default)

resulting in the token cache being bypassed.

PublicClientApplication is not affected, as application.ClientApplication._find_msal_accounts still uses the old find.

for a in self.token_cache.find(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link

@Moazzem-Hossain-pixel Moazzem-Hossain-pixel left a comment

Choose a reason for hiding this comment

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

If you can take initiative to resolve that will be highly appreciated.

@rayluo
Copy link
Collaborator Author

rayluo commented May 9, 2024

If you can take initiative to resolve that will be highly appreciated.

@Moazzem-Hossain-pixel , you are commenting on a topic that has already been implemented. There are some follow-ups in their own github issues linked above, and we are addressing them respectively. But the main change in the current topic is already stable. And our downstream partner - Azure CLI - has been unblocked by AzureAD/microsoft-authentication-extensions-for-python#127

If you are blocked by any specific issue, please add your comment into those linked issues, or even create a new issue to elaborate. Meanwhile, I'm locking this issue here.

@AzureAD AzureAD locked as resolved and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants