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

Implement IUtf8SpanParsable on IPAddress and IPNetwork #102144

Merged
merged 45 commits into from
Oct 24, 2024

Conversation

edwardneal
Copy link
Contributor

@edwardneal edwardneal commented May 13, 2024

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.

This is preparation for the work required to enable IUtf8SpanParsable support on IPAddress and IPNetwork.
Also replaced a few instances of unsafe code with spans.
This uses the same style of generic constraints as the numeric parsing in corelib (although it can't access IUtfChar<TSelf>.)
All tests pass, although the UTF8 code paths haven't yet been tested.
Some restructuring is pending as I plumb IPAddressParser to the updated generic methods.
Future commits will handle the downstream dependencies of IPvXAddressHelper - at present, this looks like Uri and TargetHostNameHelper.
This allows me to eliminate the nested class and now-redundant generic type constraints on each method
These are just the changes required to make System.Private.Uri compile.
* Split IPAddressParser and IPv6AddressHelper into multiple files and shared common components between projects.
* Updated some of the Uri-specific additions to IPvXAddressHelper files to use spans.
* Minor adjustments to project files to support the above.
Removed a using statement in the ref project, small reduction in the csproj diff for System.Net.Primitives
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

karelz commented Jun 6, 2024

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!

@karelz karelz added the blocked Issue/PR is blocked on something - see comments label Jun 6, 2024
@karelz karelz requested a review from tannergooding June 6, 2024 15:00
@edwardneal edwardneal marked this pull request as draft June 6, 2024 16:42
@MihaZupan MihaZupan marked this pull request as ready for review June 14, 2024 20:33
}
buffer[buffer.Length - 1] = '\0';

if (Interop.IpHlpApi.ConvertInterfaceNameToLuid(buffer, ref interfaceLuid) != 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* SetLastError has the correct value in the Windows interop layer.
* Preserve InterfaceNameToIndex's errno in the Unix InterfaceInfoPal.
@MihaZupan
Copy link
Member

@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
Copy link
Contributor Author

edwardneal commented Oct 6, 2024

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
Copy link
Member

EgorBo commented Oct 6, 2024

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
Copy link
Contributor Author

edwardneal commented Oct 7, 2024

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.

Prior to code review
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
Post code review

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 RatioSD Gen0 Code Size Allocated Alloc Ratio
TryParse Baseline .NET 9.0 ::192.168.0.1 81.18 ns 0.306 ns 0.271 ns 1.00 0.00 0.0031 3,341 B 80 B 1.00
TryParse CoreRun .NET 10.0 ::192.168.0.1 74.32 ns 0.815 ns 0.763 ns 0.92 0.01 0.0031 71 B 80 B 1.00
TryParse Baseline .NET 9.0 037777777777 28.85 ns 0.157 ns 0.146 ns 1.00 0.01 0.0015 1,730 B 40 B 1.00
TryParse CoreRun .NET 10.0 037777777777 25.44 ns 0.266 ns 0.249 ns 0.88 0.01 0.0016 71 B 40 B 1.00
TryParse Baseline .NET 9.0 0xff.0x7f.0x20.0x01 35.73 ns 0.309 ns 0.289 ns 1.00 0.01 0.0015 1,725 B 40 B 1.00
TryParse CoreRun .NET 10.0 0xff.0x7f.0x20.0x01 29.78 ns 0.244 ns 0.216 ns 0.83 0.01 0.0015 71 B 40 B 1.00
TryParse Baseline .NET 9.0 1:2:3:4:5:6:7:8:: 46.69 ns 0.042 ns 0.033 ns 1.00 0.00 - 1,966 B - NA
TryParse CoreRun .NET 10.0 1:2:3:4:5:6:7:8:: 44.08 ns 0.108 ns 0.090 ns 0.94 0.00 - 71 B - NA
TryParse Baseline .NET 9.0 100:0(...):abcd [22] 114.79 ns 0.407 ns 0.381 ns 1.00 0.00 0.0031 3,013 B 80 B 1.00
TryParse CoreRun .NET 10.0 100:0(...):abcd [22] 106.68 ns 0.706 ns 0.660 ns 0.93 0.01 0.0031 71 B 80 B 1.00
TryParse Baseline .NET 9.0 192.168.0.0/16 22.88 ns 0.066 ns 0.062 ns 1.00 0.00 - 1,708 B - NA
TryParse CoreRun .NET 10.0 192.168.0.0/16 22.01 ns 0.165 ns 0.155 ns 0.96 0.01 - 71 B - NA
TryParse Baseline .NET 9.0 192.168.0.1 28.77 ns 0.251 ns 0.235 ns 1.00 0.01 0.0015 1,750 B 40 B 1.00
TryParse CoreRun .NET 10.0 192.168.0.1 27.31 ns 0.282 ns 0.264 ns 0.95 0.01 0.0016 71 B 40 B 1.00
TryParse Baseline .NET 9.0 4294967295 23.37 ns 0.315 ns 0.295 ns 1.00 0.02 0.0016 1,766 B 40 B 1.00
TryParse CoreRun .NET 10.0 4294967295 23.70 ns 0.166 ns 0.155 ns 1.01 0.01 0.0016 71 B 40 B 1.00
TryParse Baseline .NET 9.0 Fe08::1%13542 75.05 ns 0.848 ns 0.793 ns 1.00 0.01 0.0048 3,415 B 120 B 1.00
TryParse CoreRun .NET 10.0 Fe08::1%13542 63.02 ns 0.704 ns 0.658 ns 0.84 0.01 0.0031 71 B 80 B 0.67
UriHost Baseline .NET 9.0 http:(...)3/foo [39] 410.91 ns 0.532 ns 0.444 ns 1.00 0.00 0.0076 17,316 B 192 B 1.00
UriHost CoreRun .NET 10.0 http:(...)3/foo [39] 364.69 ns 0.556 ns 0.520 ns 0.89 0.00 0.0076 18,560 B 192 B 1.00
UriHost Baseline .NET 9.0 http:(...)3/foo [26] 229.11 ns 0.663 ns 0.588 ns 1.00 0.00 0.0072 15,314 B 184 B 1.00
UriHost CoreRun .NET 10.0 http:(...)3/foo [26] 229.23 ns 0.923 ns 0.863 ns 1.00 0.00 0.0072 16,081 B 184 B 1.00

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

* Replaced int.CreateTruncating with a JIT pattern which converts from TChar to a ushort.
* Removed various TChar.CreateTruncating constants, reinstating direct character constants.
* Replaced branch statements when parsing IPv4 numerics with a HexConverter lookup.
* IPv4 parts stored in an array of longs to improve register usage.
* Small improvement in reassembly to eliminate extra array access.
* Reverted changes to Uri parsing layer, removing the translation logic between a Span and a pointer for IPv4 addresses.
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

* Replaced ushorts with integers to remove some zero-extensions.
* Replaced NativeMemory with an ArrayPool<byte>.
* Removed unnecessary assignment to ch.
@MihaZupan
Copy link
Member

@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;
}

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sticking through all the reviews, change & perf LGTM

@MihaZupan MihaZupan merged commit f199591 into dotnet:main Oct 24, 2024
81 of 83 checks passed
@MihaZupan MihaZupan added this to the 10.0.0 milestone Oct 24, 2024
@edwardneal
Copy link
Contributor Author

That's great, thanks for your patience @MihaZupan and everyone who reviewed. There are a few optimisations which I want to pick up from the earlier revisions, but I'll definitely re-read the earlier conversations on this PR first - I'm fairly sure I dragged the review process out by missing the point of some of them!

@edwardneal edwardneal deleted the issue-81500-ipaddress-ipnetwork branch October 24, 2024 16:55
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Implement IUtf8SpanParsable on IPAddress & IPNetwork