GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Several improvements to SocketsHttpHandler perf by stephentoub · Pull Request #41640 · dotnet/corefx

This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@stephentoub

@stephentoub stephentoub commented Oct 8, 2019

Copy link
Copy Markdown
Member

Fixes https://github.com/dotnet/corefx/issues/39835

Commit 1:
Per https://github.com/dotnet/corefx/issues/39835#issuecomment-538755366, the intent of HttpHeaders.TryAddWithoutValidation was that we wouldn't validate the headers. But while we're currently not validating them as part of that API call itself, we're currently validating them as part of enumerating the headers to write the out to the wire, for both HTTP/1.1 and HTTP/2.0. Stop doing that. This does mean that we could end up writing out something that would render the HTTP request invalid, but that's the nature of "WithoutValidation" and what the developer asked for.

Commit 2:
Several releases ago, when we weren't paying attention to ExpectContinue, we optimized away the backing field for it into a lazily-initialized collection; that made it cheaper when not accessed but more expensive when accessed, which was fine, as we wouldn't access it from the implementation and developers would rarely set it. But now SocketsHttpHandler checks it on every request, which means we're paying for the more expensive thing always. So, revert the optimization for this field.

Commit 3:
When we enumerate the headers to write them out, we currently allocate a string[] for each. We can instead just fill the same array over and over and over.

Benchmark:

Method Toolchain Mean Error StdDev Ratio Gen 0 Allocated
HttpGet \old\corerun.exe 66.44 us 0.886 us 0.828 us 1.00 0.8545 5.58 KB
HttpGet \new\corerun.exe 61.88 us 0.935 us 0.780 us 0.93 0.4883 3.15 KB
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Diagnosers;
using BenchmarkDotNet.Running;
using System;
using System.Net;
using System.Net.Http;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;

[MemoryDiagnoser]
public class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromTypes(new[] { typeof(Program) }).Run(args);

    private static Socket s_listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    private static HttpClient s_client = new HttpClient(new HttpClientHandler() { ServerCertificateCustomValidationCallback = delegate { return true; } });
    private static Uri s_uri;

    [Benchmark]
    public async Task HttpGet()
    {
        var m = new HttpRequestMessage(HttpMethod.Get, s_uri);
        m.Headers.TryAddWithoutValidation("Authorization", "ANYTHING SOMEKEY");
        m.Headers.TryAddWithoutValidation("Referer", "http://someuri.com");
        m.Headers.TryAddWithoutValidation("User-Agent", "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.90 Safari/537.36");
        m.Headers.TryAddWithoutValidation("Host", "www.somehost.com");
        (await s_client.SendAsync(m, default)).Dispose();
    }

    [GlobalSetup]
    public void CreateSocketServer()
    {
        s_listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
        s_listener.Listen(int.MaxValue);
        var ep = (IPEndPoint)s_listener.LocalEndPoint;
        s_uri = new Uri($"http://{ep.Address}:{ep.Port}/");
        byte[] response = Encoding.UTF8.GetBytes("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n");
        byte[] endSequence = new byte[] { (byte)'\r', (byte)'\n', (byte)'\r', (byte)'\n' };

        Task.Run(async () =>
        {
            while (true)
            {
                Socket s = await s_listener.AcceptAsync();
                _ = Task.Run(() =>
                {
                    using (var ns = new NetworkStream(s, true))
                    {
                        byte[] buffer = new byte[1024];
                        int totalRead = 0;
                        while (true)
                        {
                            totalRead += ns.Read(buffer.AsSpan(totalRead));
                            if (buffer.AsSpan(0, totalRead).IndexOf(endSequence) == -1)
                            {
                                if (totalRead == buffer.Length) Array.Resize(ref buffer, buffer.Length * 2);
                                continue;
                            }

                            ns.Write(response);

                            totalRead = 0;
                        }
                    }
                });
            }
        });
    }
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Rodrigo-Andrade and BlindGazer reacted with hooray emoji
All reactions
  • 🎉 2 reactions
@stephentoub stephentoub added tenet-performance Performance related issue area-System.Net.Http labels Oct 8, 2019
@stephentoub stephentoub added this to the 5.0 milestone Oct 8, 2019
@davidsh
davidsh requested a review from a team October 8, 2019 16:31
stephentoub added 2 commits October 8, 2019 13:03
@stephentoub
Do not force-parse TryAddWithoutValidation headers in SocketsHttpHandler
726763b
When enumerating headers to write them out, do not force them to be parsed if the user explicitly asked for them not to be with "WithoutValidation".  If the key/values are incorrectly formatted, the request may end up being written incorrectly on the wire, but that's up to the developer explicitly choosing it.
@stephentoub
Revert HttpRequestHeaders.ExpectContinue optimization
758b70e
Several releases ago, when we weren't paying attention to ExpectContinue, we optimized away the backing field for it into a lazily-initialized collection; that made it cheaper when not accessed but more expensive when accessed, which was fine, as we wouldn't access it from the implementation and developers would rarely set it.  But now SocketsHttpHandler checks it on every request, which means we're paying for the more expensive thing always. So, revert the optimization for this field.
@stephentoub
stephentoub force-pushed the noforceparseheaders branch from dfe8860 to 866931b Compare October 8, 2019 17:06
@scalablecory

scalablecory commented Oct 8, 2019

Copy link
Copy Markdown

First pass looks okay to me; will make a second pass later. Would love if we could avoid mixing logic changes with style changes -- reading this takes some effort :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@stephentoub

stephentoub commented Oct 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Which style changes? Just the var to the actual type? I only fixed things where I was already touching the code or where writing new code the "right" way would have left things inconsistent. If I changed something else by muscle memory 😄, I can revert it.

davidsh reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@davidsh

davidsh commented Oct 8, 2019

Copy link
Copy Markdown
Contributor

I'm curious/hopeful if this change will help with #22638.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@stephentoub
Avoid allocating a string[] per header
df25530
When we enumerate the headers to write them out, we currently allocate a string[] for each.  We can instead just fill the same array over and over and over.
@stephentoub
stephentoub force-pushed the noforceparseheaders branch from 866931b to df25530 Compare October 8, 2019 18:12
davidsh
davidsh approved these changes Oct 8, 2019
View reviewed changes
@geoffkizer

geoffkizer commented Oct 8, 2019

Copy link
Copy Markdown

Re retrieving header values: It would be nice to avoid the string[] allocation here entirely. This is particularly relevant for HTTP3 since there's no easy way to reuse the string[] across requests -- each request is on its own QUIC stream.

But I don't see an obvious way to do this. Maybe a callback model? We can defer that for now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

geoffkizer
geoffkizer approved these changes Oct 8, 2019
View reviewed changes
@stephentoub

stephentoub commented Oct 8, 2019

Copy link
Copy Markdown
Member Author

/azp run

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@azure-pipelines

azure-pipelines Bot commented Oct 8, 2019

Copy link
Copy Markdown
Azure Pipelines successfully started running 4 pipeline(s).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

scalablecory
scalablecory approved these changes Oct 8, 2019
View reviewed changes
@stephentoub
stephentoub merged commit 4d346a9 into dotnet:master Oct 9, 2019
@stephentoub
stephentoub deleted the noforceparseheaders branch October 9, 2019 01:22
@davidsh davidsh mentioned this pull request Oct 9, 2019
Uri.UnescapeDataString and Uri.UnescapeString reuse the original string when possible #41684
Merged
@stephentoub stephentoub mentioned this pull request Dec 13, 2019
Correction Accept headers dotnet/runtime#792
Closed
@stephentoub stephentoub mentioned this pull request Mar 11, 2021
TryAddWithoutValidation works differently for HttpClient.DefaultRequestHeaders and HttpRequestMessage.Headers dotnet/runtime#49463
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@stephentoub
Several improvements to SocketsHttpHandler perf (dotnet/corefx#41640)
108bf6b
* Do not force-parse TryAddWithoutValidation headers in SocketsHttpHandler

When enumerating headers to write them out, do not force them to be parsed if the user explicitly asked for them not to be with "WithoutValidation".  If the key/values are incorrectly formatted, the request may end up being written incorrectly on the wire, but that's up to the developer explicitly choosing it.

* Revert HttpRequestHeaders.ExpectContinue optimization

Several releases ago, when we weren't paying attention to ExpectContinue, we optimized away the backing field for it into a lazily-initialized collection; that made it cheaper when not accessed but more expensive when accessed, which was fine, as we wouldn't access it from the implementation and developers would rarely set it.  But now SocketsHttpHandler checks it on every request, which means we're paying for the more expensive thing always. So, revert the optimization for this field.

* Avoid allocating a string[] per header

When we enumerate the headers to write them out, we currently allocate a string[] for each.  We can instead just fill the same array over and over and over.


Commit migrated from dotnet/corefx@4d346a9
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
@scalablecory scalablecory scalablecory approved these changes
@davidsh davidsh davidsh approved these changes
@geoffkizer geoffkizer geoffkizer approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

area-System.Net.Http tenet-performance Performance related issue

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

HttpClient requests - improve header parsing perf (when TryAddWithoutValidation)

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@stephentoub @scalablecory @davidsh @geoffkizer
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.