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

RedirectToHttpsRule: opportunity for performance improvement #28826

Closed
paulomorgado opened this issue Dec 23, 2020 · 6 comments
Closed

RedirectToHttpsRule: opportunity for performance improvement #28826

paulomorgado opened this issue Dec 23, 2020 · 6 comments
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware design-proposal This issue represents a design proposal for a different issue, linked in the description help wanted Up for grabs. We would accept a PR to help resolve this issue Perf
Milestone

Comments

@paulomorgado
Copy link
Contributor

Summary

While debugging some code, I noticed that RedirectToHttpsRule is instantiating a StringBuilder and boxing structs on every request.

Motivation and goals

This is in a hot path for applications that use redirection to HTTPS.

Detailed design

Most uses of HostString, PathString and QueryString use ToString() or ToUriComponent() to avoid boxing. This is not the case for RedirectToHttpsRule:

new StringBuilder().Append("https://").Append(host).Append(req.PathBase).Append(req.Path).Append(req.QueryString);

So, I started playing with options:

[MemoryDiagnoser]
public class Benchmark
{
    private static readonly string[] hosts = new[] { "cname.domain.tld" };
    private static readonly string[] basePaths = new[] { null, "/base-path", };
    private static readonly string[] paths = new[] { "/", "/path/one/two/three", };
    private static readonly string[] queries = new[] { null, "?param1=value1&param2=value2&param3=value3", };

    public IEnumerable<object[]> Data()
    {
        foreach (var host in hosts)
        {
            foreach (var basePath in basePaths)
            {
                foreach (var path in paths)
                {
                    foreach (var query in queries)
                    {
                        yield return new object[] { new HostString(host), new PathString(basePath), new PathString(path), new QueryString(query), };
                    }
                }
            }
        }
    }

    [Benchmark(Baseline = true)]
    [ArgumentsSource(nameof(Data))]
    public string StringBuilderWithBoxing(HostString host, PathString basePath, PathString path, QueryString query)
        => new StringBuilder()
            .Append("https://")
            .Append(host)
            .Append(basePath)
            .Append(path)
            .Append(query)
            .ToString();

    [Benchmark]
    [ArgumentsSource(nameof(Data))]
    public string StringBuilderWithoutBoxing(HostString host, PathString basePath, PathString path, QueryString query)
        => new StringBuilder()
            .Append("https://")
            .Append(host.ToUriComponent())
            .Append(basePath.ToUriComponent())
            .Append(path.ToUriComponent())
            .Append(query.ToUriComponent())
        .ToString();

    [Benchmark]
    [ArgumentsSource(nameof(Data))]
    public string StringFormatWithBoxing(HostString host, PathString basePath, PathString path, QueryString query)
        => $"https://{host}{basePath}{path}{query}";

    [Benchmark]
    [ArgumentsSource(nameof(Data))]
    public string StringFormatWithoutBoxing(HostString host, PathString basePath, PathString path, QueryString query)
        => $"https://{host.ToUriComponent()}{basePath.ToUriComponent()}{path.ToUriComponent()}{query.ToUriComponent()}";

    [Benchmark]
    [ArgumentsSource(nameof(Data))]
    public string StringConcat(HostString host, PathString basePath, PathString path, QueryString query)
    {
        if (!basePath.HasValue)
        {
            return string.Concat("https://", host, path.ToUriComponent(), query.ToUriComponent());
        }

        if (!query.HasValue)
        {
            return string.Concat("https://", host, basePath.ToUriComponent(), path.ToUriComponent());
        }

        return string.Concat("https://", host, basePath.ToUriComponent(), path.ToUriComponent(), query.ToUriComponent());
    }

    [Benchmark]
    [ArgumentsSource(nameof(Data))]
    public string Crazy1(HostString host, PathString basePath, PathString path, QueryString query)
    {
        var uriParts = (
            scheme: "https://",
            host: host.ToUriComponent(),
            basePath: basePath.ToUriComponent(),
            path: path.ToUriComponent(),
            query: query.ToUriComponent());

        var length = uriParts.scheme.Length + uriParts.host.Length + uriParts.basePath.Length + uriParts.path.Length + uriParts.query.Length;

        return string.Create(
            length,
            uriParts,
            (buffer, uriParts) =>
            {
                var i = -1;

                foreach (var c in uriParts.scheme)
                {
                    buffer[++i] = c;
                }

                foreach (var c in uriParts.host)
                {
                    buffer[++i] = c;
                }

                if (uriParts.basePath.Length != 0)
                {
                    foreach (var c in uriParts.basePath)
                    {
                        buffer[++i] = c;
                    }
                }

                foreach (var c in uriParts.path)
                {
                    buffer[++i] = c;
                }

                if (uriParts.query.Length != 0)
                {
                    foreach (var c in uriParts.query)
                    {
                        buffer[++i] = c;
                    }
                }
            });
    }

    [Benchmark]
    [ArgumentsSource(nameof(Data))]
    public string Crazy2(HostString host, PathString basePath, PathString path, QueryString query)
    {
        const string httpsSchemePrefix = "https://";
        const int httpsSchemePrefixLength = 8;
        Debug.Assert(httpsSchemePrefix.Length == httpsSchemePrefixLength, "{nameof(httpsSchemePrefixLength)} should be {httpsSchemePrefix.Length} and is {httpsSchemePrefixLength}");

        var uriParts = (
            host: host.ToUriComponent(),
            basePath: basePath.ToUriComponent(),
            path: path.ToUriComponent(),
            query: query.ToUriComponent());

        var length = httpsSchemePrefixLength + uriParts.host.Length + uriParts.basePath.Length + uriParts.path.Length + uriParts.query.Length;

        return string.Create(
            length,
            uriParts,
            (buffer, uriParts) =>
            {
                var span = httpsSchemePrefix.AsSpan();
                span.CopyTo(buffer);

                var i = httpsSchemePrefixLength;

                span = uriParts.host.AsSpan();
                span.CopyTo(buffer.Slice(i, span.Length));
                i += span.Length;

                if (uriParts.basePath.Length != 0)
                {
                    span = uriParts.basePath.AsSpan();
                    span.CopyTo(buffer.Slice(i, span.Length));
                    i += span.Length;
                }

                span = uriParts.path.AsSpan();
                span.CopyTo(buffer.Slice(i, span.Length));
                i += span.Length;

                if (uriParts.query.Length != 0)
                {
                    span = uriParts.query.AsSpan();
                    span.CopyTo(buffer.Slice(i, span.Length));
                }
            });
    }
}

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.21277
Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.200-preview.20614.14
[Host] : .NET Core 5.0.1 (CoreCLR 5.0.120.57516, CoreFX 5.0.120.57516), X64 RyuJIT
DefaultJob : .NET Core 5.0.1 (CoreCLR 5.0.120.57516, CoreFX 5.0.120.57516), X64 RyuJIT

Method host basePath path query Mean Error StdDev Median Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
StringBuilderWithBoxing cname.domain.tld / 382.9 ns 38.31 ns 112.95 ns 369.1 ns 1.00 0.00 0.0782 - - 328 B
StringBuilderWithoutBoxing cname.domain.tld / 304.8 ns 23.13 ns 68.19 ns 296.4 ns 0.84 0.24 0.0668 - - 280 B
StringFormatWithBoxing cname.domain.tld / 495.3 ns 43.01 ns 126.83 ns 483.5 ns 1.43 0.59 0.0534 - - 224 B
StringFormatWithoutBoxing cname.domain.tld / 147.4 ns 7.46 ns 21.41 ns 140.6 ns 0.42 0.11 0.0324 - - 136 B
StringConcat cname.domain.tld / 220.3 ns 14.88 ns 42.94 ns 212.0 ns 0.65 0.26 0.0496 - - 208 B
Crazy1 cname.domain.tld / 145.2 ns 8.58 ns 24.90 ns 138.5 ns 0.42 0.15 0.0172 - - 72 B
Crazy2 cname.domain.tld / 133.6 ns 7.20 ns 20.67 ns 126.7 ns 0.39 0.15 0.0172 - - 72 B
StringBuilderWithBoxing cname.domain.tld / ?para(...)alue3 [42] 400.0 ns 18.60 ns 53.95 ns 392.5 ns 1.00 0.00 0.1335 - - 560 B
StringBuilderWithoutBoxing cname.domain.tld / ?para(...)alue3 [42] 408.3 ns 20.84 ns 60.80 ns 401.0 ns 1.04 0.20 0.1221 - - 512 B
StringFormatWithBoxing cname.domain.tld / ?para(...)alue3 [42] 455.5 ns 19.01 ns 54.86 ns 438.0 ns 1.16 0.21 0.0744 - - 312 B
StringFormatWithoutBoxing cname.domain.tld / ?para(...)alue3 [42] 253.8 ns 15.62 ns 45.33 ns 238.8 ns 0.65 0.15 0.0534 - - 224 B
StringConcat cname.domain.tld / ?para(...)alue3 [42] 261.8 ns 9.77 ns 28.35 ns 249.8 ns 0.67 0.12 0.0706 - - 296 B
Crazy1 cname.domain.tld / ?para(...)alue3 [42] 261.8 ns 11.49 ns 33.53 ns 256.8 ns 0.67 0.13 0.0381 - - 160 B
Crazy2 cname.domain.tld / ?para(...)alue3 [42] 191.9 ns 6.41 ns 17.86 ns 188.1 ns 0.49 0.07 0.0381 - - 160 B
StringBuilderWithBoxing cname.domain.tld /path/one/two/three 515.1 ns 26.82 ns 78.23 ns 494.3 ns 1.00 0.00 0.1202 - - 504 B
StringBuilderWithoutBoxing cname.domain.tld /path/one/two/three 420.0 ns 12.85 ns 36.46 ns 408.7 ns 0.83 0.13 0.1087 - - 456 B
StringFormatWithBoxing cname.domain.tld /path/one/two/three 603.6 ns 36.37 ns 105.50 ns 587.3 ns 1.19 0.23 0.0629 - - 264 B
StringFormatWithoutBoxing cname.domain.tld /path/one/two/three 285.5 ns 8.47 ns 24.04 ns 278.8 ns 0.56 0.09 0.0420 - - 176 B
StringConcat cname.domain.tld /path/one/two/three 364.6 ns 14.04 ns 41.41 ns 356.2 ns 0.72 0.13 0.0591 - - 248 B
Crazy1 cname.domain.tld /path/one/two/three 277.0 ns 5.65 ns 14.19 ns 273.2 ns 0.56 0.08 0.0267 - - 112 B
Crazy2 cname.domain.tld /path/one/two/three 272.4 ns 6.98 ns 19.92 ns 264.9 ns 0.54 0.09 0.0267 - - 112 B
StringBuilderWithBoxing cname.domain.tld /path/one/two/three ?para(...)alue3 [42] 618.1 ns 27.22 ns 78.54 ns 600.5 ns 1.00 0.00 0.1869 - - 784 B
StringBuilderWithoutBoxing cname.domain.tld /path/one/two/three ?para(...)alue3 [42] 555.5 ns 11.16 ns 29.20 ns 546.8 ns 0.92 0.10 0.1755 - - 736 B
StringFormatWithBoxing cname.domain.tld /path/one/two/three ?para(...)alue3 [42] 656.4 ns 33.39 ns 96.88 ns 621.5 ns 1.08 0.21 0.0820 - - 344 B
StringFormatWithoutBoxing cname.domain.tld /path/one/two/three ?para(...)alue3 [42] 413.8 ns 21.86 ns 62.71 ns 393.0 ns 0.68 0.11 0.0610 - - 256 B
StringConcat cname.domain.tld /path/one/two/three ?para(...)alue3 [42] 456.0 ns 19.25 ns 55.84 ns 446.0 ns 0.75 0.12 0.0782 - - 328 B
Crazy1 cname.domain.tld /path/one/two/three ?para(...)alue3 [42] 407.0 ns 11.24 ns 31.52 ns 396.5 ns 0.67 0.08 0.0458 - - 192 B
Crazy2 cname.domain.tld /path/one/two/three ?para(...)alue3 [42] 376.2 ns 16.68 ns 48.40 ns 361.1 ns 0.62 0.10 0.0458 - - 192 B
StringBuilderWithBoxing cname.domain.tld /base-path / 390.5 ns 15.75 ns 46.21 ns 375.9 ns 1.00 0.00 0.1163 - - 488 B
StringBuilderWithoutBoxing cname.domain.tld /base-path / 380.4 ns 15.97 ns 45.56 ns 372.0 ns 0.99 0.16 0.1049 - - 440 B
StringFormatWithBoxing cname.domain.tld /base-path / 499.0 ns 32.56 ns 92.36 ns 472.7 ns 1.30 0.27 0.0591 - - 248 B
StringFormatWithoutBoxing cname.domain.tld /base-path / 258.8 ns 11.54 ns 33.67 ns 255.7 ns 0.67 0.11 0.0381 - - 160 B
StringConcat cname.domain.tld /base-path / 249.8 ns 6.38 ns 17.90 ns 246.0 ns 0.65 0.07 0.0553 - - 232 B
Crazy1 cname.domain.tld /base-path / 202.6 ns 4.14 ns 6.56 ns 200.1 ns 0.52 0.06 0.0229 - - 96 B
Crazy2 cname.domain.tld /base-path / 200.9 ns 8.15 ns 23.63 ns 194.8 ns 0.52 0.08 0.0229 - - 96 B
StringBuilderWithBoxing cname.domain.tld /base-path / ?para(...)alue3 [42] 556.7 ns 31.99 ns 91.78 ns 534.3 ns 1.00 0.00 0.1831 - - 768 B
StringBuilderWithoutBoxing cname.domain.tld /base-path / ?para(...)alue3 [42] 502.9 ns 20.48 ns 56.42 ns 484.2 ns 0.92 0.17 0.1717 - - 720 B
StringFormatWithBoxing cname.domain.tld /base-path / ?para(...)alue3 [42] 586.8 ns 43.96 ns 126.82 ns 540.6 ns 1.09 0.31 0.0782 - - 328 B
StringFormatWithoutBoxing cname.domain.tld /base-path / ?para(...)alue3 [42] 429.7 ns 35.01 ns 103.24 ns 382.2 ns 0.80 0.24 0.0572 - - 240 B
StringConcat cname.domain.tld /base-path / ?para(...)alue3 [42] 360.8 ns 8.61 ns 24.30 ns 359.3 ns 0.66 0.11 0.0782 - - 328 B
Crazy1 cname.domain.tld /base-path / ?para(...)alue3 [42] 391.9 ns 32.32 ns 95.28 ns 349.5 ns 0.72 0.19 0.0420 - - 176 B
Crazy2 cname.domain.tld /base-path / ?para(...)alue3 [42] 262.8 ns 5.18 ns 10.23 ns 259.4 ns 0.51 0.07 0.0420 - - 176 B
StringBuilderWithBoxing cname.domain.tld /base-path /path/one/two/three 636.3 ns 53.84 ns 158.75 ns 565.9 ns 1.00 0.00 0.1240 - - 520 B
StringBuilderWithoutBoxing cname.domain.tld /base-path /path/one/two/three 623.6 ns 46.91 ns 133.84 ns 584.1 ns 1.07 0.35 0.1125 - - 472 B
StringFormatWithBoxing cname.domain.tld /base-path /path/one/two/three 611.5 ns 16.20 ns 45.96 ns 595.5 ns 1.04 0.23 0.0668 - - 280 B
StringFormatWithoutBoxing cname.domain.tld /base-path /path/one/two/three 365.9 ns 12.68 ns 34.93 ns 353.5 ns 0.63 0.10 0.0458 - - 192 B
StringConcat cname.domain.tld /base-path /path/one/two/three 494.8 ns 40.47 ns 119.34 ns 485.9 ns 0.82 0.27 0.0629 - - 264 B
Crazy1 cname.domain.tld /base-path /path/one/two/three 356.8 ns 6.76 ns 17.69 ns 350.8 ns 0.63 0.11 0.0305 - - 128 B
Crazy2 cname.domain.tld /base-path /path/one/two/three 374.2 ns 19.14 ns 54.91 ns 356.5 ns 0.62 0.13 0.0305 - - 128 B
StringBuilderWithBoxing cname.domain.tld /base-path /path/one/two/three ?para(...)alue3 [42] 900.7 ns 75.84 ns 223.62 ns 808.0 ns 1.00 0.00 0.1926 - - 808 B
StringBuilderWithoutBoxing cname.domain.tld /base-path /path/one/two/three ?para(...)alue3 [42] 700.5 ns 41.28 ns 119.75 ns 646.0 ns 0.82 0.24 0.1812 - - 760 B
StringFormatWithBoxing cname.domain.tld /base-path /path/one/two/three ?para(...)alue3 [42] 1,013.9 ns 73.98 ns 218.14 ns 1,015.0 ns 1.21 0.43 0.0877 - - 368 B
StringFormatWithoutBoxing cname.domain.tld /base-path /path/one/two/three ?para(...)alue3 [42] 484.2 ns 21.74 ns 60.24 ns 469.1 ns 0.56 0.14 0.0668 - - 280 B
StringConcat cname.domain.tld /base-path /path/one/two/three ?para(...)alue3 [42] 725.7 ns 59.97 ns 176.81 ns 728.3 ns 0.86 0.32 0.0877 - - 368 B
Crazy1 cname.domain.tld /base-path /path/one/two/three ?para(...)alue3 [42] 643.1 ns 53.89 ns 158.89 ns 579.0 ns 0.77 0.30 0.0515 - - 216 B
Crazy2 cname.domain.tld /base-path /path/one/two/three ?para(...)alue3 [42] 406.6 ns 6.35 ns 10.78 ns 405.9 ns 0.36 0.06 0.0515 - - 216 B
  • As the table shows, and, as expect, boxing cause more memory to be allocated than not boxing.
  • Using string.Concat, as it is, is not always faster or allocates less memory than using string.Format without boxing.
  • The Crazy options are always faster and use allocate less memory. Crazy2 is the fastest option.
@paulomorgado paulomorgado added the design-proposal This issue represents a design proposal for a different issue, linked in the description label Dec 23, 2020
@mkArtakMSFT mkArtakMSFT added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-routing labels Dec 24, 2020
@Tratcher Tratcher added area-middleware and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-routing labels Dec 28, 2020
@Tratcher
Copy link
Member

Tratcher commented Dec 28, 2020

RedirectToHttpsRule should be updated to use UriHelper.BuildAbsolute and we can focus any optimization efforts over there. It already has a few improvements over this code.

@paulomorgado
Copy link
Contributor Author

Yes! And I'm working on a proposal to improve the performance of that too.

@BrennanConroy BrennanConroy added this to the Backlog milestone Dec 28, 2020
@ghost
Copy link

ghost commented Dec 28, 2020

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.

@BrennanConroy BrennanConroy added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Dec 28, 2020
@paulomorgado
Copy link
Contributor Author

I think this could also be improvde:

if (SSLPort.HasValue && SSLPort.Value > 0)
{
    // a specific SSL port is specified
    host = new HostString(host.Host, SSLPort.Value);
}
else
{
    // clear the port
    host = new HostString(host.Host);
}

If the `SSLPort was sanitized in a constructor:

public RedirectToHttpsRule(int statusCode, int? sslPort)
{
    StatusCode = statusCode;
    SSLPort = (sslPort.HasValue && sslPort.GetValueOrDefault() > 0) ? sslPort.GetValueOrDefault() : 0;
}

public int StatusCode{ get; }
public int SSLPort { get; }

That code could become:

if (SSLPort > 0)
{
    // a specific SSL port is specified
    host = new HostString(host.Host, SSLPort);
}
else
{
    // clear the port
    host = new HostString(host.Host);
}

Less memory used and less processing time.

@paulomorgado
Copy link
Contributor Author

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

@paulomorgado
Copy link
Contributor Author

Found! MiddlewareTests.cs

@BrennanConroy BrennanConroy modified the milestones: Backlog, 6.0-preview4 Apr 6, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2021
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware design-proposal This issue represents a design proposal for a different issue, linked in the description help wanted Up for grabs. We would accept a PR to help resolve this issue Perf
Projects
None yet
Development

No branches or pull requests

6 participants