Return all local IPs on Linux #41764

ManickaP · 2019-10-14T14:29:43Z

As per discussion in the #32611, for local host name return also getifaddrs IPs and not just getaddrinfo ones.
The current fix has been benchmarked against the master.

Method	hostName	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated
GetHostAddresses new	manicka-pc	49,736.0 ns	475.718 ns	444.986 ns	49,494.6 ns	0.0610	-	-	432 B
GetHostAddresses old	manicka-pc	7,567.3 ns	147.82 ns	145.18 ns	7,572.7 ns	0.0305	-	-	168 B

The fixed version is ~40us slower. This is an intentional performance regression in order to make the behavior more consistent across OSes.

Fixes #32611

Complete numbers

Explanation

The original GetHostAddresses only called getaddrinfo which essentially either takes data from etc/hosts (getaddrinfo for microsoft.com) or does a DNS query (getaddrinfo for ibm.com). You can clearly see the performance difference in between those two (10us vs 100ms). For local host getaddrinfo usually returned only the single IP listed in etc/hosts, which was the main difference between Windows and Linux version (see original issue #32611). Yes, you can manually add more addresses to the file and they will be returned. I guess it can do a DNS query for local host name as well depending on the nsswitch.conf. Tough the proposed solution was to add the IP addresses of all network interfaces (via getifaddrs) if GetHostAddresses is called for the local host name. So the performance regression is premeditated and has been measured in the original issue. This can be seen on the line with GetHostAddresses for manicka-pc, the number there shows that the method, in that particular case, has slowed and the difference corresponds to the system call of getiffaddrs (3rd line from the top).

I also ran PerfCollect on the fixed version and did direct time measurement with Stopwatch on the master and fixed version and haven't found any obvious performance problems. Actually, the fixed version was a bit faster in the ParseHostEntry. Probably since it doesn't fetch and convert IP address one by one through interop, but rather get's them all at once.

The fix:

Method	hostName	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated
GetHostName	?	515.0 ns	3.339 ns	2.788 ns	514.2 ns	0.0105	-	-	48 B
Adapters	?	426,529.4 ns	1,572.995 ns	1,471.381 ns	426,984.1 ns	41.5039	-	-	175856 B
getifaddrs	?	38,262.4 ns	326.961 ns	305.839 ns	38,169.9 ns	0.0610	-	-	384 B
GetHostAddresses	ibm.com	160,548,952.9 ns	40,232,817.945 ns	113,477,376.838 ns	133,815,611.3 ns	-	-	-	72 B
getaddrinfo	ibm.com	121,347,416.4 ns	30,647,103.141 ns	87,932,283.906 ns	94,461,035.3 ns	-	-	-	168 B
GetHostAddresses	manicka-pc	49,736.0 ns	475.718 ns	444.986 ns	49,494.6 ns	0.0610	-	-	432 B
getaddrinfo	manicka-pc	6,516.5 ns	17.000 ns	14.196 ns	6,516.5 ns	0.0381	-	-	168 B
GetHostAddresses	microsoft.com	7,264.0 ns	32.249 ns	30.166 ns	7,261.7 ns	0.0153	-	-	72 B
getaddrinfo	microsoft.com	6,689.2 ns	30.766 ns	24.020 ns	6,688.9 ns	0.0381	-	-	168 B

The master:

Method	hostName	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated
GetHostName	?	558.0 ns	10.93 ns	13.42 ns	551.7 ns	0.0114	-	-	48 B
Adapters	?	471,709.3 ns	9,209.42 ns	14,063.76 ns	467,884.4 ns	41.5039	-	-	175688 B
getifaddrs	?	41,697.4 ns	818.68 ns	1,005.41 ns	41,385.4 ns	0.0610	-	-	384 B
GetHostAddresses	ibm.com	31,173,248.4 ns	2,757,460.81 ns	7,264,254.71 ns	30,084,315.6 ns	-	-	-	168 B
getaddrinfo	ibm.com	34,948,165.7 ns	3,831,926.31 ns	10,294,221.19 ns	31,398,652.5 ns	-	-	-	168 B
GetHostAddresses	manicka-pc	7,567.3 ns	147.82 ns	145.18 ns	7,572.7 ns	0.0305	-	-	168 B
getaddrinfo	manicka-pc	7,568.3 ns	151.08 ns	398.00 ns	7,510.9 ns	0.0381	-	-	168 B
GetHostAddresses	microsoft.com	7,310.1 ns	137.50 ns	147.12 ns	7,284.0 ns	0.0381	-	-	168 B
getaddrinfo	microsoft.com	7,024.3 ns	139.63 ns	155.20 ns	7,011.7 ns	0.0381	-	-	168 B

ManickaP · 2019-10-14T14:30:23Z

/azp run

azure-pipelines · 2019-10-14T14:30:45Z

Azure Pipelines successfully started running 4 pipeline(s).

danmoseley · 2019-10-15T04:20:15Z

How should I read the table? It seems to read that "fix" is slower (160,548,952.9 vs 31,173,248.4)

wfurt · 2019-10-15T05:32:30Z

The interesting case is GetHostAddresses/manicka-pc. ibm.com will depend on external servers and results will be unreliable. We start measuring them as bulk compassion to local processing.

src/Native/Unix/System.Native/pal_networking.c

stephentoub · 2019-10-15T17:38:59Z

src/Native/Unix/System.Native/pal_networking.c

+                ConvertInAddrToByteArray(addressList->Address, NUM_BYTES_IN_IPV4_ADDRESS, &inetSockAddr->sin_addr);
+                addressList->IsIPv6 = 0;
+                ++addressList;
+                continue;


Why use continue; in both of these blocks rather than just making the next if into an else if?

Because I like the symmetricity of it and I'm not a fan of if {} else if {} without final else {}. However, if you think that if {} else if {} is generally more readable I'll gladly change it.

seems like the block are very similar. You may get away with ternary operator in few places where it differs.

stephentoub · 2019-10-15T17:49:15Z

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

@@ -70,19 +70,15 @@ private static unsafe void ParseHostEntry(Interop.Sys.HostEntry hostEntry, bool
                    // is likely to involve extra allocations, hashing, etc., and so will probably be more expensive than
                    // this one in the typical (short list) case.

-                    var nativeAddresses = new Interop.Sys.IPAddress[hostEntry.IPAddressCount];
+                    Interop.Sys.IPAddress* nativeAddresses = stackalloc Interop.Sys.IPAddress[(int)hostEntry.AddressCount];


Can we use spans instead of adding unsafe code?

Also, how do we know that the AddressCount will be small enough for this to go on the stack?

Can we use spans instead of adding unsafe code?

I agree we shouldn't be needing to add unsafe code anymore now that Span<T> and friends are available.

This was a mistake, I lost some changes somewhere in between the multiple repo clones I have. Here should have been Span and lower Slice.IndexOf.

Anyway, I've changed it back to dynamic allocation. According to DNS RFC, it may use TCP (ch 4.2.2) to transmit the data, thus effectively bypassing the UDP limit for the datagram size (ch 2.3.4). So I cannot safely say that it will be small enough to fit on the stack.

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

ManickaP · 2019-10-16T07:47:18Z

@danmosemsft

How should I read the table? It seems to read that "fix" is slower (160,548,952.9 vs 31,173,248.4)

I updated the PR summary, picked the important numbers and added explanation for the complete benchmark tables.

ManickaP · 2019-10-16T11:20:59Z

/azp run

azure-pipelines · 2019-10-16T11:21:23Z

Azure Pipelines successfully started running 4 pipeline(s).

ManickaP · 2019-10-16T11:25:18Z

@stephentoub Could you please explicitly state whether you are OK with the performance regression brought by this change?

I've updated the description to be more readable:

The fixed version is ~40us slower. This is an intentional performance regression in order to make the behavior more consistent across OSes.

wfurt

few comment. generally looks good.

src/Native/Unix/System.Native/pal_networking.c

src/Common/src/Interop/Unix/System.Native/Interop.HostEntry.cs

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

src/Native/Unix/System.Native/pal_networking.c

wfurt · 2019-10-16T17:47:23Z

src/Native/Unix/System.Native/pal_networking.c

+                ConvertInAddrToByteArray(addressList->Address, NUM_BYTES_IN_IPV4_ADDRESS, &inetSockAddr->sin_addr);
+                addressList->IsIPv6 = 0;
+                ++addressList;
+                continue;


seems like the block are very similar. You may get away with ternary operator in few places where it differs.

wfurt · 2019-10-16T18:01:40Z

src/Native/Unix/System.Native/pal_networking.c

-}
+#if HAVE_GETIFADDRS
+    char name[MAX_HOST_NAME + 1];
+    result = gethostname((char*)name, MAX_HOST_NAME);


We will call this for every lookup, right? e.g. we will slow down common case when trying to resolve names other than ours. Since it is unlikely this will change during process run it may be worth of some caching.

The same behavior, without caching, is on Mono.

stephentoub · 2019-10-17T01:14:59Z

Could you please explicitly state whether you are OK with the performance regression brought by this change?

Functional correctness is more important than performance. So the question is really, is this change necessary for functional correctness? What user scenarios break if we don't do it?

If it is necessary, are there use cases where the perf hit measurably impacts real usage? e.g. if someone is doing microservices between processes on the same machine and for some reason ends up creating a new connection for each request, will this show up?

ManickaP · 2019-10-22T08:59:19Z

/azp run

azure-pipelines · 2019-10-22T08:59:44Z

Azure Pipelines successfully started running 4 pipeline(s).

geoffkizer · 2019-10-23T06:08:01Z

I think the perf hit is reasonable here.

If it is necessary, are there use cases where the perf hit measurably impacts real usage? e.g. if someone is doing microservices between processes on the same machine and for some reason ends up creating a new connection for each request, will this show up?

If you're doing something like this, it seems like you'd probably use "localhost" or loopback addresses rather than the actual host name.

# Conflicts: # src/Native/Unix/System.Native/pal_networking.c

ManickaP · 2019-10-29T13:35:38Z

@wfurt @stephentoub Could any of you please either do another round of review or approve this?

src/Native/Unix/System.Native/pal_networking.c

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

src/Native/Unix/System.Native/pal_networking.c

ManickaP · 2019-10-31T13:17:44Z

/azp run

azure-pipelines · 2019-10-31T13:18:08Z

Azure Pipelines successfully started running 4 pipeline(s).

stephentoub

LGTM

…/corefx#41764) Unifies behavior of GetHostAddresses for local host name on Linux with Windows and Mono versions by adding the result of getifaddrs. Commit migrated from dotnet/corefx@d8cfa3b

ManickaP added 12 commits October 1, 2019 17:21

Added interop for getfiaddrs.

8713443

HostInterfaces parsed in C# Unix PAL.

c69c736

Fixed linked files in csproj.

99de10f

Fixed compilation.

c767b71

Fixed test project.

a2bb451

Merged getifaddrs into GetHostEntryForName

64d6052

pal_networking returns IPAddress array instead of original lists.

dba4a46

GetHostName to C pal, fixed missing free.

4a9a7d4

Reverted accidental formatting change, clean up.

6698096

Formatting

a9719af

Merge branch 'master' into mapichov/32611_getifaddrs

c6fbee4

Fixed NameResolution tests.

21eb284

Dotnet-GitSync-Bot added the area-System.Net label Oct 14, 2019

ManickaP self-assigned this Oct 14, 2019

ManickaP requested a review from a team October 14, 2019 14:30

ManickaP added the os-linux Linux OS (any supported distro) label Oct 14, 2019

Fixed OSX compilation

cdc83f3

davidsh added this to the 5.0 milestone Oct 14, 2019

davidsh requested a review from stephentoub October 14, 2019 16:53

stephentoub reviewed Oct 15, 2019

View reviewed changes

Addressing PR comments

4d1682f

wfurt reviewed Oct 16, 2019

View reviewed changes

ManickaP added 2 commits October 17, 2019 15:50

Addressed PR comments

110a24e

Try to mimic mono behavior.

f06697e

Merge branch 'master' into mapichov/32611_getifaddrs

5142544

# Conflicts: # src/Native/Unix/System.Native/pal_networking.c