-
Notifications
You must be signed in to change notification settings - Fork 401
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
Reduce string allocations by comparing issuers in-place #2597
Conversation
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
I'd really like to see a measurement that shows allocations |
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were no tests to update?
There were lots of E2E style tests that covered this.. I could write some additional unit tests just for added comfort. Could never hurt! |
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadIssuerValidator/AadIssuerValidator.cs
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Validators/AadTokenValidationParametersExtension.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets at least get the missing test case. Pls evaluate the rest of the comments.
…y-identitymodel-extensions-for-dotnet into kellysong/string-replace
Fixes #2595
Benchmarks for changes for AadIssuerValidator
BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3593/23H2/2023Update/SunValley3) (Hyper-V)
AMD EPYC 7763, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-preview.3.24204.13
[Host] : .NET 8.0.5 (8.0.524.21615), X64 RyuJIT AVX2
MediumRun : .NET 8.0.5 (8.0.524.21615), X64 RyuJIT AVX2
Job=MediumRun MaxAbsoluteError=10.0000 ms IterationCount=15
LaunchCount=4 WarmupCount=10
Benchmarks for changes for AadTokenValidationParametersExtension
BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3593/23H2/2023Update/SunValley3) (Hyper-V)
AMD EPYC 7763, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-preview.3.24204.13
[Host] : .NET 8.0.5 (8.0.524.21615), X64 RyuJIT AVX2
MediumRun : .NET 8.0.5 (8.0.524.21615), X64 RyuJIT AVX2
Job=MediumRun MaxAbsoluteError=10.0000 ms IterationCount=15
LaunchCount=4 WarmupCount=10