Implement IUtf8SpanParsable on IPAddress and IPNetwork

[edwardneal]

Closes #103111

Relates to #81500. cc @tannergooding - hopefully this doesn't tread on any prior work.

This implements IUtf8SpanParsable on IPAddress and IPNetwork. It does so by making IPAddressParser generic between byte and char. Doing so needed deeper changes than I'd have liked (the separators aren't considered constants, which means that I had to swap a switch statement and its "goto case" statements for a set of if statements) As I did that, I replaced most of the unsafe code and pointers with ReadOnlySpans (which eliminates a pattern where downstream code was pinning an ROS and passing its pointer.)

IPAddressParser, IPv4AddressHelper and IPv6AddressHelper are shared with System.Net.Quic, System.Net.Security and System.Private.Uri. The changes to the first two projects are pretty minor, consisting of one change to TargetHostNameHelper. System.Private.Uri is similar, but it makes more extensive use of pointers.

There are two differences between this PR and the underlying issue:

• I've added the public methods Parse(ReadOnlySpan<byte>) and TryParse(ReadOnlySpan<byte>, out IPNetwork) to IPNetwork.
• IPNetwork implements IUtf8SpanParsable explicitly rather than implicitly.

In both cases, this is done to maintain consistency with the ISpanParsable members which were already implemented. I've taken this to be covered by the original API review.

It's worth noting that IPv6AddressHelper<TChar>.IsValid in System.Private.Uri continues to reference char* rather than ReadOnlySpan<TChar>. This is inconsistent, but I've left it alone for now. I'd prefer to address that in a follow-up PR which replaces the unsafe components from Uri.PrivateParseMinimal and the methods it calls with a ROS.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[tannergooding]

Hi @edwardneal, sorry for the delay in getting to this, I had missed the tag initially since this is under the networking area.

It's worth noting that IPAddress and IPNetwork are not part of APIs that #81500 was approved for, and so before this can move forward there needs to be a new API proposal that specifically asks for implementing IUtf8SpanParsable on these types.

That proposal should be straightforward and I expect have no issues going through API review, at which point this can likely get merged. If you could assist in opening such a proposal, that should help the process go along faster.

[edwardneal]

Thanks @tannergooding. Sorry for the confusion here - I read the original issue as approving the addition of IUtf8SpanParsable and IUtf8SpanFormattable to anything already implementing ISpanFormattable, and IPAddress and IPNetwork both do so. Should I just raise the API proposal for the additional IPNetwork.Parse(ReadOnlySpan<byte>) and IPNetwork.TryParse(ReadOnlySpan<byte>, out IPNetwork) methods and IPNetwork's explicit interface implementation, or should I also include IPAddress in that?

[tannergooding]

should I also include IPAddress in that?

We want to include all new public APIs and interfaces in the proposal whether that is for IUtf8SpanParsable, IUtf8SpanFormattable, or both; as well as any overloads beyond this.

I read the original issue as approving the addition of IUtf8SpanParsable and IUtf8SpanFormattable to anything already implementing ISpanFormattable

The general spirit behind the feature fits that, but we still do explicit review and sign-off as there are often edge cases that we may warrant different consideration.

[karelz]

Given that we are blocked on API review, I would recommend to either change the PR to Draft or close it, until the API is approved. @edwardneal what do you prefer and can you please do it? Thanks!

[ManickaP]

I added the NO MERGE label. Once we branch out, I'll remove it and this could be merged (mid August).

[liveans]

Deleted NO-MERGE label since branching for .NET 9 has been done.

[stephentoub]

If this produces an error, but in the finally we then Free the native memory, will that end up corrupting whatever error code the caller might need to retrieve?

[edwardneal]

It won't on Windows: the documentation doesn't say that any of the methods changes the result of GetLastError, and testing confirms this - the return value is the only success/failure response, so there's no error code to corrupt. SetLastError was set to true on the interop definitions though, I've corrected this.

It will on Unix, so I've wrapped the Free in GetLastPInvokeError/SetLastPInvokeError as per the pattern in CriticalHandle.Cleanup.

[stephentoub]

Thanks! Left a few comments/questions, but otherwise LGTM.

[MihaZupan]

@EgorBot

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

public class Bench
{
    [Benchmark]
    [Arguments("192.168.0.1")]
    [Arguments("4294967295")]
    [Arguments("037777777777")]
    [Arguments("0xff.0x7f.0x20.0x01")]
    [Arguments("192.168.0.0/16")]
    [Arguments("::192.168.0.1")]
    [Arguments("100:0:1:2:0:0:000:abcd")]
    [Arguments("Fe08::1%13542")]
    [Arguments("1:2:3:4:5:6:7:8::")]
    public bool TryParse(string s) => IPAddress.TryParse(s, out _);

    [Benchmark]
    [Arguments("http://192.168.0.1:123/foo")]
    [Arguments("http://[100:0:1:2:0:0:000:abcd]:123/foo")]
    public string UriHost(string s) => new Uri(s).Host;
}

[edwardneal]

Thanks MihaZupan. That's quite a wide regression in ::192.168.0.1, and I don't see a clear reason why. Some performance is left on the table in HexConverter.FromChar and similar (it doesn't cast its input to a uint, so the JIT doesn't elide the bounds check and we end up performing it twice) but we were using that method anyway - just indirectly, as a result of calling Uri.FromHex. In any event though, this method isn't called in the slower case - that test case's code path hasn't actually changed.

I've reverted the only other difference so that there's a common baseline, and I'll keep looking at this. I'm faintly suspicious that passing around an int rather than a char is causing the JIT to implicitly reintroduce bounds checks, but I wouldn't expect them to have such a large impact in this case.

[EgorBo]

Some performance is left on the table in HexConverter.FromChar and similar (it doesn't cast its input to a uint, so the JIT doesn't elide the bounds check and we end up performing it twice)

Can you point me exactly to the non-elided bound check? Last I checked, all users of FromChar give it something JIT can see it's never negative. E.g. Uri.FromHex passes char to HexConverter.FromChar, hence, never negative.

[edwardneal]

I agree. FromChar is defined as

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int FromChar(int c)
{
    return c >= CharToHexLookup.Length ? 0xFF : CharToHexLookup[c];
}

c isn't cast to an unsigned type before the comparison to CharToHexLookup.Length, so if the caller passes it an int then a bounds check is performed.

That's not normally a problem - like you said, Uri.FromHex passes it a char, so when it's inlined it makes perfect sense that the JIT can see which datatype is really being passed around. I'm passing it an integer though, so the bounds check would likely remain.

I'm hesitant to say that it's the only cause of the performance regression because when I benchmarked it, the bounds checks only introduced a 0.2ns/call performance gap, even on a slower laptop. I'm going to test the idea over the next few days (edit: this'll likely be the weekend now, sorry.)

---

Updating on this: investigating the performance regressions led to a few places where CreateTruncating of a non-constant value performed a few nanoseconds slower than I'd expected, in the middle of a number of hot loops. I've replaced this with a separate ToUShort method, which performs a direct cast.

The UriHost benchmarks were stranger. As best I can tell, IPv4AddressHelper.Parse's resultant assembly was 200 bytes smaller when JITed under the PR environment than when JITed under .NET 9.0 RC2. This led to a number of different inlining decisions. I've brought the PR up to date with the main branch and re-run the benchmarks... they look roughly acceptable to me.

BenchmarkDotNet v0.14.0, Windows 10 (10.0.20348.2582) (Hyper-V)
Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK 9.0.100-rc.2.24474.11
  [Host]   : .NET 9.0.0 (9.0.24.47305), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  Baseline : .NET 9.0.0 (9.0.24.47305), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  CoreRun  : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

Method	Job	Runtime	s	Mean	Error	StdDev	Ratio	Gen0	Code Size	Allocated	Alloc Ratio
TryParse	Baseline	.NET 9.0	::192.168.0.1	83.40 ns	0.421 ns	0.393 ns	1.00	0.0031	3,363 B	80 B	1.00
TryParse	CoreRun	.NET 10.0	::192.168.0.1	82.96 ns	0.256 ns	0.227 ns	0.99	0.0031	71 B	80 B	1.00
TryParse	Baseline	.NET 9.0	037777777777	29.75 ns	0.194 ns	0.182 ns	1.00	0.0015	1,730 B	40 B	1.00
TryParse	CoreRun	.NET 10.0	037777777777	26.48 ns	0.111 ns	0.104 ns	0.89	0.0016	71 B	40 B	1.00
TryParse	Baseline	.NET 9.0	0xff.0x7f.0x20.0x01	36.47 ns	0.122 ns	0.108 ns	1.00	0.0015	1,725 B	40 B	1.00
TryParse	CoreRun	.NET 10.0	0xff.0x7f.0x20.0x01	31.04 ns	0.179 ns	0.167 ns	0.85	0.0015	71 B	40 B	1.00
TryParse	Baseline	.NET 9.0	1:2:3:4:5:6:7:8::	46.71 ns	0.041 ns	0.037 ns	1.00	-	1,996 B	-	NA
TryParse	CoreRun	.NET 10.0	1:2:3:4:5:6:7:8::	44.48 ns	0.074 ns	0.066 ns	0.95	-	71 B	-	NA
TryParse	Baseline	.NET 9.0	100:0(...):abcd [22]	116.31 ns	0.318 ns	0.282 ns	1.00	0.0031	3,013 B	80 B	1.00
TryParse	CoreRun	.NET 10.0	100:0(...):abcd [22]	109.84 ns	0.174 ns	0.163 ns	0.94	0.0031	71 B	80 B	1.00
TryParse	Baseline	.NET 9.0	192.168.0.0/16	22.89 ns	0.052 ns	0.049 ns	1.00	-	1,708 B	-	NA
TryParse	CoreRun	.NET 10.0	192.168.0.0/16	22.29 ns	0.147 ns	0.130 ns	0.97	-	71 B	-	NA
TryParse	Baseline	.NET 9.0	192.168.0.1	29.99 ns	0.226 ns	0.211 ns	1.00	0.0015	1,750 B	40 B	1.00
TryParse	CoreRun	.NET 10.0	192.168.0.1	28.78 ns	0.202 ns	0.179 ns	0.96	0.0015	71 B	40 B	1.00
TryParse	Baseline	.NET 9.0	4294967295	24.58 ns	0.104 ns	0.097 ns	1.00	0.0016	1,766 B	40 B	1.00
TryParse	CoreRun	.NET 10.0	4294967295	24.98 ns	0.189 ns	0.177 ns	1.02	0.0016	71 B	40 B	1.00
TryParse	Baseline	.NET 9.0	Fe08::1%13542	78.23 ns	0.186 ns	0.174 ns	1.00	0.0048	3,413 B	120 B	1.00
TryParse	CoreRun	.NET 10.0	Fe08::1%13542	65.86 ns	0.224 ns	0.210 ns	0.84	0.0031	71 B	80 B	0.67
UriHost	Baseline	.NET 9.0	http:(...)3/foo [39]	415.16 ns	0.485 ns	0.454 ns	1.00	0.0076	17,319 B	192 B	1.00
UriHost	CoreRun	.NET 10.0	http:(...)3/foo [39]	373.66 ns	1.215 ns	1.077 ns	0.90	0.0076	17,769 B	192 B	1.00
UriHost	Baseline	.NET 9.0	http:(...)3/foo [26]	227.89 ns	0.270 ns	0.252 ns	1.00	0.0072	15,314 B	184 B	1.00
UriHost	CoreRun	.NET 10.0	http:(...)3/foo [26]	228.84 ns	0.269 ns	0.224 ns	1.00	0.0072	16,081 B	184 B	1.00

Pending any surprises from CI, I'll re-request the benchmark from EgorBot.