Remove allocations from Dns.* by stephentoub · Pull Request #41061 · dotnet/corefx

stephentoub · 2019-09-12T17:31:15Z

This started as an effort to reduce the size of System.Net.NameResolution.dll when publishing a trimmed app. It's not that big to begin with, but it's carrying around a copy of all of the IAsyncResult helper types, because the Get*Async methods are currently wrappers for the Begin/End* methods.

This PR inverts that, wrapping the Begin/End* methods instead around the Get*Async methods, using the same TaskToApm helper we use in other places in corefx for the same purpose. This makes the Get*Async methods faster and lighter weight, but it does increase the number/amount of allocation in the Begin/End* APIs. Since these are considered legacy, I normally would consider that a good trade, however we still use these Begin/End methods in a few places in System.Net.Sockets.dll (which should be fixed later), and I didn't want to regress those use cases.

So, this also then trims some additional fat, which helps the Get*Async cases even further, and gets the Begin/End* to be even better than before the change. This includes not allocating an IPHostEntry when we're just going to unwrap it and return its addresses, computing the exact IPAddress[] size we need rather than using a List<> to grow it and ToArray to create the actual array, avoiding creating the HostName if we don't need it, avoiding an unnecessary SafeHandle allocation, and avoiding a closure allocation in the TaskToApm helpers.

As part of this, I also noticed that we had some bugs in how some of our interop structures on Windows were defined. In particular, fields that in the native types were size_t were defined as int rather than IntPtr in the managed code (this appears to be carry-over from when the types were only used with the 32-bit variants). It appears we've been saved from corruption by padding, but I fixed it, regardless.

And as long as I was changing pretty much everything else, where I was touching code I also cleaned up some legacy style stuff.

Size:
In an app published as trimmed that just uses HttpClient to download a page, prior to this change, System.Net.NameResolution.dll was 30K; after this change it's 24K.

Perf:

Method	Toolchain	Mean	Error	Allocated
GetHostEntry_Name	New	140,461.01 ns	1,506.485 ns	200 B
GetHostEntry_Name	Old	140,554.96 ns	234.500 ns	328 B

GetHostAddresses_Name	New	139,344.50 ns	470.858 ns	120 B
GetHostAddresses_Name	Old	144,846.26 ns	2,773.511 ns	328 B

GetHostAddresses_Address	New	66.67 ns	0.355 ns	72 B
GetHostAddresses_Address	Old	69.09 ns	0.082 ns	72 B

GetHostEntryAsync_Name	New	202,342.23 ns	1,192.177 ns	394 B
GetHostEntryAsync_Name	Old	205,828.40 ns	1,580.557 ns	736 B

GetHostAddressesAsync_Name	New	201,192.37 ns	392.106 ns	312 B
GetHostAddressesAsync_Name	Old	206,888.85 ns	1,571.552 ns	736 B

GetHostAddressesAsync_Address	New	74.95 ns	0.240 ns	144 B
GetHostAddressesAsync_Address	Old	255.51 ns	0.659 ns	448 B

BeginEndGetHostEntry_Name	New	202,922.99 ns	1,825.023 ns	472 B
BeginEndGetHostEntry_Name	Old	205,842.96 ns	1,115.982 ns	496 B

BeginEndGetHostAddresses_Name	New	202,684.50 ns	510.180 ns	392 B
BeginEndGetHostAddresses_Name	Old	208,066.47 ns	2,733.112 ns	498 B

BeginEndGetHostAddresses_Address	New	117.19 ns	0.543 ns	176 B
BeginEndGetHostAddresses_Address	Old	216.04 ns	1.604 ns	272 B

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Net;
using System.Threading;
using System.Threading.Tasks;

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

    [Benchmark] public IPHostEntry GetHostEntry_Name() => Dns.GetHostEntry("bing.com");

    [Benchmark] public IPAddress[] GetHostAddresses_Name() => Dns.GetHostAddresses("bing.com");
    [Benchmark] public IPAddress[] GetHostAddresses_Address() => Dns.GetHostAddresses("13.107.21.200");

    [Benchmark] public Task GetHostEntryAsync_Name() => Dns.GetHostEntryAsync("bing.com");

    [Benchmark] public Task GetHostAddressesAsync_Name() => Dns.GetHostAddressesAsync("bing.com");
    [Benchmark] public Task GetHostAddressesAsync_Address() => Dns.GetHostAddressesAsync("13.107.21.200");

    [Benchmark]
    public void BeginEndGetHostEntry_Name()
    {
        _mre.Reset();
        Dns.BeginGetHostEntry("bing.com", iar =>
        {
            Dns.EndGetHostEntry(iar);
            ((ManualResetEventSlim)iar.AsyncState).Set();
        }, _mre);
        _mre.Wait();
    }

    [Benchmark]
    public void BeginEndGetHostAddresses_Name()
    {
        _mre.Reset();
        Dns.BeginGetHostAddresses("bing.com", iar =>
        {
            Dns.EndGetHostAddresses(iar);
            ((ManualResetEventSlim)iar.AsyncState).Set();
        }, _mre);
        _mre.Wait();
    }

    [Benchmark]
    public void BeginEndGetHostAddresses_Address()
    {
        _mre.Reset();
        Dns.BeginGetHostAddresses("13.107.21.200", iar =>
        {
            Dns.EndGetHostAddresses(iar);
            ((ManualResetEventSlim)iar.AsyncState).Set();
        }, _mre);
        _mre.Wait();
    }
}

src/Common/src/CoreLib/System/Threading/Tasks/TaskToApm.cs

src/System.Net.NameResolution/src/System/Net/DNS.cs

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

This started as an effort to reduce the size of System.Net.NameResolution.dll when publishing a trimmed app. It's not that big to begin with, but it's carrying around a copy of all of the IAsyncResult helper types, because the Get*Async methods are currently wrappers for the Begin/End* methods. This PR inverts that, wrapping the Begin/End* methods instead around the Get*Async methods, using the same TaskToApm helper we use in other places in corefx for the same purpose. This makes the Get*Async methods faster and lighterweight, but it does increase the number/amount of allocation in the Begin/End* APIs. Since these are considered legacy, I normally would consider that a good trade, however we still use these Begin/End methods in a few places in System.Net.Sockets, and I didn't want to regress those use cases. So, this also then trims some additional fat, which helps the Get*Async cases even further, and gets the Begin/End* to be even better than before the change. This includes not allocating an IPHostEntry when we're just going to unwrap it and return its addresses, computing the exact IPAddress[] size we need rather than using a List<> to grow it and ToArray to create the actual array, avoiding creating the HostName if we don't need it, and avoiding an unnecessary SafeHandle allocation. As part of this, I also noticed that we had some bugs in how some of our interop structures on Windows were defined. In particular, fields that in the native types were size_t were defined as int rather than IntPtr in the managed code. It appears we were saved by padding, but I fixed it regardless. And as long as I was changing pretty much everything else, where I was touching code I also cleaned up some legacy style stuff.

This started as an effort to reduce the size of System.Net.NameResolution.dll when publishing a trimmed app. It's not that big to begin with, but it's carrying around a copy of all of the IAsyncResult helper types, because the Get*Async methods are currently wrappers for the Begin/End* methods. This PR inverts that, wrapping the Begin/End* methods instead around the Get*Async methods, using the same TaskToApm helper we use in other places in corefx for the same purpose. This makes the Get*Async methods faster and lighterweight, but it does increase the number/amount of allocation in the Begin/End* APIs. Since these are considered legacy, I normally would consider that a good trade, however we still use these Begin/End methods in a few places in System.Net.Sockets, and I didn't want to regress those use cases. So, this also then trims some additional fat, which helps the Get*Async cases even further, and gets the Begin/End* to be even better than before the change. This includes not allocating an IPHostEntry when we're just going to unwrap it and return its addresses, computing the exact IPAddress[] size we need rather than using a List<> to grow it and ToArray to create the actual array, avoiding creating the HostName if we don't need it, and avoiding an unnecessary SafeHandle allocation. As part of this, I also noticed that we had some bugs in how some of our interop structures on Windows were defined. In particular, fields that in the native types were size_t were defined as int rather than IntPtr in the managed code. It appears we were saved by padding, but I fixed it regardless. And as long as I was changing pretty much everything else, where I was touching code I also cleaned up some legacy style stuff. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

This started as an effort to reduce the size of System.Net.NameResolution.dll when publishing a trimmed app. It's not that big to begin with, but it's carrying around a copy of all of the IAsyncResult helper types, because the Get*Async methods are currently wrappers for the Begin/End* methods. This PR inverts that, wrapping the Begin/End* methods instead around the Get*Async methods, using the same TaskToApm helper we use in other places in corefx for the same purpose. This makes the Get*Async methods faster and lighterweight, but it does increase the number/amount of allocation in the Begin/End* APIs. Since these are considered legacy, I normally would consider that a good trade, however we still use these Begin/End methods in a few places in System.Net.Sockets, and I didn't want to regress those use cases. So, this also then trims some additional fat, which helps the Get*Async cases even further, and gets the Begin/End* to be even better than before the change. This includes not allocating an IPHostEntry when we're just going to unwrap it and return its addresses, computing the exact IPAddress[] size we need rather than using a List<> to grow it and ToArray to create the actual array, avoiding creating the HostName if we don't need it, and avoiding an unnecessary SafeHandle allocation. As part of this, I also noticed that we had some bugs in how some of our interop structures on Windows were defined. In particular, fields that in the native types were size_t were defined as int rather than IntPtr in the managed code. It appears we were saved by padding, but I fixed it regardless. And as long as I was changing pretty much everything else, where I was touching code I also cleaned up some legacy style stuff. Commit migrated from dotnet/corefx@a55e95c

stephentoub added tenet-performance Performance related issue area-System.Net labels Sep 12, 2019

stephentoub added this to the 5.0 milestone Sep 12, 2019

jkotas reviewed Sep 12, 2019

View reviewed changes

src/Common/src/CoreLib/System/Threading/Tasks/TaskToApm.cs Outdated Show resolved Hide resolved

jkotas reviewed Sep 12, 2019

View reviewed changes

src/Common/src/CoreLib/System/Threading/Tasks/TaskToApm.cs Outdated Show resolved Hide resolved

stephentoub force-pushed the dnstasks branch from fdc2ab7 to bb24957 Compare September 12, 2019 21:28