RedirectRule: opportunity for performance improvement

Summary

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

Motivation and goals

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

Detailed design

Use path.HasValue instead.
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.

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.

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); }

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); 
        
           }

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.

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.

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.

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

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

Found! MiddlewareTests.cs

Navigation Menu

Repository navigation

RedirectRule: opportunity for performance improvement #28883

Summary

Motivation and goals

Detailed design

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

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

	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);
	}

RedirectRule: opportunity for performance improvement #28883

Description

Summary

Motivation and goals

Detailed design

Activity

Tratcher commented on Dec 28, 2020

paulomorgado commented on Dec 28, 2020

Tratcher commented on Dec 28, 2020

paulomorgado commented on Dec 28, 2020

Tratcher commented on Dec 28, 2020

paulomorgado commented on Dec 29, 2020

paulomorgado commented on Dec 29, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions