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.

Sign up for GitHub

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

Jump to bottom

Reduce string allocations by comparing issuers in-place #2597

Merged
merged 16 commits into from
Jun 5, 2024
Merged

Reduce string allocations by comparing issuers in-place #2597

merged 16 commits into from
Jun 5, 2024

Conversation

kellyyangsong
Copy link
Contributor

@kellyyangsong kellyyangsong commented May 14, 2024 •
edited

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

Method Mean Error StdDev Median P90 P95 P100 Gen0 Allocated
Before 53.03 ns 0.482 ns 1.048 ns 52.65 ns 54.76 ns 55.20 ns 56.80 ns 0.0086 144 B
After 20.88 ns 0.026 ns 0.059 ns 20.87 ns 20.94 ns 20.97 ns 21.09 ns - -

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

Method Mean Error StdDev P90 P95 P100 Gen0 Allocated
Before 86.88 ns 0.542 ns 1.202 ns 88.61 ns 89.01 ns 90.18 ns 0.0162 272 B
After 36.93 ns 0.345 ns 0.751 ns 37.80 ns 38.10 ns 40.37 ns 0.0081 136 B

@kellyyangsong kellyyangsong requested a review from a team as a code owner May 14, 2024 23:28
@keegan-caruso
Copy link
Contributor

I'd really like to see a measurement that shows allocations

@kellyyangsong kellyyangsong marked this pull request as draft May 15, 2024 18:00
@kellyyangsong kellyyangsong marked this pull request as ready for review May 29, 2024 02:16
jennyf19
jennyf19 approved these changes May 30, 2024
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit: There were no tests to update?

@kellyyangsong
Copy link
Contributor Author

:shipit: 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!

jennyf19
jennyf19 approved these changes Jun 4, 2024
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

FuPingFranco
FuPingFranco approved these changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@FuPingFranco FuPingFranco left a comment

Choose a reason for hiding this comment

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

LGTM!

keegan-caruso
keegan-caruso requested changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@keegan-caruso keegan-caruso left a comment •
edited

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.

@kellyyangsong kellyyangsong merged commit dc15328 into dev Jun 5, 2024
6 checks passed
@kellyyangsong kellyyangsong deleted the kellysong/string-replace branch June 5, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brentschmaltz brentschmaltz brentschmaltz left review comments

@eerhardt eerhardt eerhardt left review comments

@victorm-hernandez victorm-hernandez victorm-hernandez left review comments

@jennyf19 jennyf19 jennyf19 approved these changes

@FuPingFranco FuPingFranco FuPingFranco approved these changes

@keegan-caruso keegan-caruso keegan-caruso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Reduce allocations in Aad validation by not using string.Replace when it isn't necessary
7 participants
@kellyyangsong @keegan-caruso @brentschmaltz @eerhardt @victorm-hernandez @jennyf19 @FuPingFranco