RedirectRule: opportunity for performance improvement

[paulomorgado]

Summary

1. RedirectRule.ApplyRule is comparing the request path with PathString.Empty, which ends up being a string comparison.
2. RedirectRule.ApplyRule is boxing a PathString(1)(2).

Motivation and goals

This is in a hot path for applications that use redirections.

Detailed design

1. Use path.HasValue instead.
2. Use pathBase.ToUriComponent() instead.
   • A better option would be use UriHelper.BuildRelative. The current implementation doesn't encode the path; only the path base and the query string.

[Tratcher]

Have you done any profiling here? This is a class of issue that's unlikely to be addressed unless it shows up as significant in a CPU or memory profile.

1. HasValue is a similar string comparison (string.IsNullOrEmpty).
   

   aspnetcore/src/Http/Http.Abstractions/src/PathString.cs

   Lines 47 to 49 in e04dee0

   	public bool HasValue
   	{
   	get { return !string.IsNullOrEmpty(Value); }

2. Are you sure about the boxing? PathString has an implicit string converter that may kick in first. It also overrides the + operator.
   

   aspnetcore/src/Http/Http.Abstractions/src/PathString.cs

   Lines 454 to 455 in e04dee0

   	public static implicit operator string(PathString path)
   	=> path.ToString();

   
   

   aspnetcore/src/Http/Http.Abstractions/src/PathString.cs

   Lines 414 to 419 in e04dee0

   	public static string operator +(PathString left, string? right)
   	{
   	// This overload exists to prevent the implicit string<->PathString converter from
   	// trying to call the PathString+PathString operator for things that are not path strings.
   	return string.Concat(left.ToString(), right);
   	}

[paulomorgado]

If you follow the PathString equality operator, you'll see that it's more than just string.IsNullOrEmpty. And then there's consistency with the rest of the codebase.

You're right about the string.Concat overload being used. It's string.Concat(string, string, string) - there are overloads up to 4 string arguments and the C# compiler prefers the implicit conversion and the overload with string argumetns.

However, the PathString addition operator doesn't apply because the type of the next argument is a sting, not a PathString. It should be, because it's not being encoded like the rest of the components.

[Tratcher]

However, the PathString addition operator doesn't apply because the type of the next argument is a sting, not a PathString. It should be, because it's not being encoded like the rest of the components.

It also has an overload for PathString + string.

[paulomorgado]

Got it!

But that means that

pathBase + newPath.Substring(0, split) + query.ToUriComponent()

will end up being

string.Concat(string.Concat(pathBase.ToUriComponent(), newPath.Substring(0, split)), query.ToUriComponent())

Right?

One more string than it needs to be. If we accept that newPath.Substring(0, split) is unavoidable.

[Tratcher]

Fair point. As you said, UriHelper.BuildRelative would address most of this.

[paulomorgado]

Are there any unit tests for this? If so, where?

[paulomorgado]

Found! MiddlewareTests.cs

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.