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

[Bug] Evaluate usage of DateTime.TryParse for performance #2615

Closed
2 of 14 tasks
eerhardt opened this issue May 28, 2024 · 1 comment · Fixed by #2622
Closed
2 of 14 tasks

[Bug] Evaluate usage of DateTime.TryParse for performance #2615

eerhardt opened this issue May 28, 2024 · 1 comment · Fixed by #2622
Assignees
Milestone

Comments

@eerhardt
Copy link
Contributor

Which version of Microsoft.IdentityModel are you using?
latest version.

Where is the issue?

  • M.IM.JsonWebTokens
  • M.IM.KeyVaultExtensions
  • M.IM.Logging
  • M.IM.ManagedKeyVaultSecurityKey
  • M.IM.Protocols
  • M.IM.Protocols.OpenIdConnect
  • M.IM.Protocols.SignedHttpRequest
  • M.IM.Protocols.WsFederation
  • M.IM.TestExtensions
  • M.IM.Tokens
  • M.IM.Tokens.Saml
  • M.IM.Validators
  • M.IM.Xml
  • S.IM.Tokens.Jwt
  • Other (please describe)

Repro

Looking at a CPU profile of validating JWT tokens, between 5-8% of the time is spent in 2 calls to DateTime.TryParse:

internal static string GetStringClaimValueType(string str)
{
if (DateTime.TryParse(str, out DateTime dateTimeValue))

For every string claim in the JWT, we are calling DateTime.TryParse on each value, whether it is expected to be a DateTime or not. This is expensive. We should consider if can eliminate these calls, or only call them if necessary.

image

image

@brentschmaltz
Copy link
Member

brentschmaltz commented May 29, 2024

@eerhardt Excellent find, this was added for backcompat.
We should state what claims we will consider as a possible datetime, ignore the rest.

I think we need to wrap this in a AppContext switch as we could break users.
We are investigating an API that will give users the ability to step in front of property claim.
When that API is ready, users will be able to handle specific claims.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants