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

Use SearchValues throughout dotnet/aspnetcore (part 3) #54878

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

ladeak
Copy link
Contributor

@ladeak ladeak commented Apr 1, 2024

Use SearchValues throughout dotnet/aspnetcore (part 3)

TagBuilder using SearchValues.
UrlResolutionTagHelper using SearchValues.

Both are tasks of #46484

Description TagBuilder

  • Adding further tests and a new benchmark to compare before-after performance
  • Updating the implementation of TagBuilder to use SearchValues instead manual operation

Performance comparison TagBuilder

Using TagBuilderBenchmark and

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.22631
12th Gen Intel Core i7-1255U, 1 CPU, 12 logical and 10 physical cores
.NET SDK=9.0.100-preview.3.24161.2
[Host] : .NET 9.0.0 (9.0.24.17301), X64 RyuJIT
Job-YBJGDV : .NET 9.0.0 (9.0.24.16003), X64 RyuJIT

Server=True Toolchain=.NET Core 9.0 RunStrategy=Throughput

BEFORE:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
ValidFieldName 23.61 ns 0.219 ns 0.194 ns 42,361,985.3 - - - -
InvalidFieldName 63.85 ns 0.506 ns 0.473 ns 15,662,151.8 0.0023 - - 208 B

AFTER:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
ValidFieldName 2.670 ns 0.0691 ns 0.0577 ns 374,546,269.6 - - - -
InvalidFieldName 37.499 ns 0.2485 ns 0.2203 ns 26,667,383.3 0.0023 - - 208 B

Description UrlResolutionTagHelper

  • Adding further tests to cover all whitespace characters in question
  • Updating the CreateTrimmed method of UrlResolutionTagHelper to use SearchValues. Could not rely on ROS as public APIs of IUriHelper requires a string.

Performance comparison UrlResolutionTagHelper

Although I have compared perf before-after the changes, for the optimistic path when there are no excess the whitespaces, the perf remained the same (gains are negligible). With extra whitespaces there is some perf gain, but the exact measurement would depend on the actual implementation of IUriHelper so the results are only "relative" (in my perf tests I used a FakeUriHelper). Hence, I have not committed the benchmark file nor included the results.

Fixes #46484

@ladeak ladeak requested a review from a team as a code owner April 1, 2024 16:05
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 1, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 1, 2024
@ladeak ladeak marked this pull request as draft April 1, 2024 17:40
@ladeak ladeak changed the title # Use SearchValues throughout dotnet/aspnetcore (part 3) Use SearchValues throughout dotnet/aspnetcore (part 3) Apr 1, 2024
@ladeak ladeak marked this pull request as ready for review April 1, 2024 17:44
ladeak added 2 commits April 3, 2024 22:07
- Adding further tests and a new benchmark to compare before-after performance
- Updating the implementation of TagBuilder to use SearchValues instead manual operation
@ladeak ladeak force-pushed the ladeak-46484-searchvalues-tagbuild branch from c437b88 to 8e7b24a Compare April 3, 2024 20:07
@ladeak ladeak marked this pull request as draft April 4, 2024 22:14
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

There are still a couple outstanding questions, but things LGTM. Thanks!

@ladeak ladeak marked this pull request as ready for review April 5, 2024 21:10
@amcasey amcasey enabled auto-merge (squash) April 5, 2024 21:43
@amcasey amcasey merged commit 26869f2 into dotnet:main Apr 5, 2024
26 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview4 milestone Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use SearchValues throughout dotnet/aspnetcore
3 participants