Skip to content

Avoid some substring allocations during Uri host normalization #117289

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

Merged
merged 2 commits into from
Jul 4, 2025

Conversation

MihaZupan
Copy link
Member

Avoids 1 or 2 temporary string allocations - the argument to Normalize and (sometimes) its result.
Only applies to Uris with non-ASCII values.

Method Toolchain Mean Error Ratio Allocated Alloc Ratio
NewUri main 397.0 ns 1.59 ns 1.00 440 B 1.00
NewUri pr 377.6 ns 1.60 ns 0.95 368 B 0.84
[MemoryDiagnoser(false)]
public class UriBench
{
    [Benchmark]
    public Uri NewUri() => new("http://some.host.with.ümlauts/");
}

@MihaZupan MihaZupan added this to the 10.0.0 milestone Jul 3, 2025
@MihaZupan MihaZupan self-assigned this Jul 3, 2025
@MihaZupan MihaZupan marked this pull request as ready for review July 3, 2025 20:30
@Copilot Copilot AI review requested due to automatic review settings July 3, 2025 20:30
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reduces temporary string allocations during URI host normalization by introducing a span-based normalization helper and optimizing Bidi control character stripping.

  • Introduces NormalizeAndConcat to normalize a span and concatenate it without intermediate allocations.
  • Refactors StripBidiControlCharacters to return a boolean and optional output, avoiding new strings when no characters are removed.
  • Updates Uri.CheckAuthorityHelper calls to use the new helpers.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/libraries/System.Private.Uri/src/System/UriHelper.cs Added NormalizeAndConcat and refactored StripBidiControlCharacters overloads for allocation improvements.
src/libraries/System.Private.Uri/src/System/Uri.cs Updated host normalization logic to use NormalizeAndConcat and new StripBidiControlCharacters overloads.
Comments suppressed due to low confidence (3)

src/libraries/System.Private.Uri/src/System/UriHelper.cs:23

  • Add unit tests for NormalizeAndConcat to verify behavior with null start values, empty spans, and spans containing non-ASCII characters to ensure correct concatenation and normalization.
        public static string NormalizeAndConcat(string? start, ReadOnlySpan<char> toNormalize)

src/libraries/System.Private.Uri/src/System/UriHelper.cs:610

  • Add unit tests for the new StripBidiControlCharacters overloads to cover both the fast path (no removal) and removal path, including the backingString fallback behavior.
        public static string StripBidiControlCharacters(ReadOnlySpan<char> strToClean, string? backingString = null)

src/libraries/System.Private.Uri/src/System/UriHelper.cs:23

  • [nitpick] Consider adding XML documentation to NormalizeAndConcat to explain its purpose, expected parameters (including null handling), and behavior for consumers and future maintainers.
        public static string NormalizeAndConcat(string? start, ReadOnlySpan<char> toNormalize)

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

@MihaZupan MihaZupan enabled auto-merge (squash) July 4, 2025 11:58
@MihaZupan MihaZupan merged commit fab69a4 into dotnet:main Jul 4, 2025
76 of 87 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants