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

Add span-based Uri {Try}{Un}EscapeDataString overloads #98074

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Feb 7, 2024

Closes #40603

namespace System
{
    partial class Uri
    {
        public static string EscapeDataString(ReadOnlySpan<char> charsToEscape);
        public static bool TryEscapeDataString(ReadOnlySpan<char> charsToEscape, Span<char> destination, out int charsWritten);

        public static string UnescapeDataString(ReadOnlySpan<char> charsToUnescape);
        public static bool TryUnescapeDataString(ReadOnlySpan<char> charsToUnescape, Span<char> destination, out int charsWritten);
    }
}

The test coverage for these was not great, so I added a bunch more combinations. I also moved similar test cases from System.Runtime.Tests into the Uri project.

I updated FormUrlEncodedContent to also use these, so it's now allocating a lot less. For a random usage I pulled from eShopOnWeb tests:

Method Toolchain Mean Error Ratio Allocated Alloc Ratio
EShopOnWebLogin main 513.7 ns 1.69 ns 1.00 2080 B 1.00
EShopOnWebLogin pr 291.3 ns 1.47 ns 0.57 592 B 0.28

Looking at something like YARP's EncodePath helper with ASP.NET Core's test paths, the difference is

Method Scenario Mean Error Ratio Allocated Alloc Ratio
Old LongTestPath 95.51 ns 0.702 ns 1.00 400 B 1.00
New LongTestPath 70.58 ns 0.291 ns 0.74 344 B 0.86
Old LongT(...)rcent [24] 337.00 ns 2.713 ns 1.00 392 B 1.00
New LongT(...)rcent [24] 198.49 ns 0.892 ns 0.59 280 B 0.71
Old TestPath 67.28 ns 0.439 ns 1.00 112 B 1.00
New TestPath 44.98 ns 0.129 ns 0.67 56 B 0.50

@MihaZupan MihaZupan added this to the 9.0.0 milestone Feb 7, 2024
@MihaZupan MihaZupan requested a review from a team February 7, 2024 00:45
@ghost
Copy link

ghost commented Feb 7, 2024

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

Issue Details

Implements #40603

namespace System
{
    partial class Uri
    {
        public static string EscapeDataString(ReadOnlySpan<char> charsToEscape);
        public static bool TryEscapeDataString(ReadOnlySpan<char> charsToEscape, Span<char> destination, out int charsWritten);

        public static string UnescapeDataString(ReadOnlySpan<char> charsToUnescape);
        public static bool TryUnescapeDataString(ReadOnlySpan<char> charsToUnescape, Span<char> destination, out int charsWritten);
    }
}

The test coverage for these was not great, so I added a bunch more combinations. I also moved similar test cases from System.Runtime.Tests into the Uri project.

Author: MihaZupan
Assignees: -
Labels:

area-System.Net

Milestone: 9.0.0

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned MihaZupan Feb 7, 2024
}

return HttpRuleParser.DefaultHttpEncoding.GetBytes(builder.ToString());
// We know the encoded length because the input is all ASCII.
Copy link
Member

Choose a reason for hiding this comment

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

How do we know the input is ASCII?

Copy link
Member Author

Choose a reason for hiding this comment

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

The input is a bunch of concatenated Uri.EscapeDataString results, which are always ASCII, and the encoding is Latin1.
I clarified it in the comment now.

@MihaZupan MihaZupan merged commit f4f9bd5 into dotnet:main Feb 13, 2024
111 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Add new overload to Uri.EscapeDataString
2 participants