-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Remove unsafe code from IPAddress #110824
Conversation
Tagging subscribers to this area: @dotnet/ncl |
This comment was marked as resolved.
This comment was marked as resolved.
src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
@EgorBot -amd -arm using System.Net;
using BenchmarkDotNet.Attributes;
[MemoryDiagnoser]
public class IPAddressBenchmarks
{
static IPAddress Ipv6_1 = IPAddress.Parse("3cca:65e1:957d:e712:1c8c:ec01:6d44:febc");
static IPAddress Ipv6_2 = IPAddress.Parse("3cca:65e1:957d:e712:1c8c:ec01:6d44:febc");
static IPAddress Ipv6_3 = IPAddress.Parse("17e1:891a:1a00:d475:92cd:d4c7:5b30:775a");
static IPAddress Ipv6_4 = IPAddress.Parse("33.55.77.99").MapToIPv6();
static IPAddress Ipv4_1 = IPAddress.Parse("33.55.77.99");
static byte[] _buffer = new byte[16];
[Benchmark]
public bool CompareSameIPv6() => Ipv6_1.Equals(Ipv6_2);
[Benchmark]
public bool CompareDifferentIPv6() => Ipv6_1.Equals(Ipv6_3);
[Benchmark]
public IPAddress MapToIPv6() => Ipv4_1.MapToIPv6();
[Benchmark]
public bool IsIPv4MappedToIPv6() => Ipv6_4.IsIPv4MappedToIPv6;
[Benchmark]
public bool TryWriteBytes_v6() => Ipv6_1.TryWriteBytes(_buffer, out _);
[Benchmark]
public IPAddress NewIPAddressv6() => new("1111222233334444"u8);
[Benchmark]
public IPAddress ParseIPAddressv6() => IPAddress.Parse("3cca:65e1:957d:e712:1c8c:ec01:6d44:febc");
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
/ba-g "Build browser-wasm linux Release LibraryTests stuck" |
{ | ||
// For IPv4 addresses, compare the integer representation. | ||
return comparand.PrivateAddress == PrivateAddress; | ||
// We give JIT a hint that the arrays are always of length IPv6AddressShorts, so it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Debug.Assert
have a side effect on JIT decisions? Even in Release
builds? Is it mentioned somewhere or is it not guaranteed? What if the assertion no longer holds in Release
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
Debug.Assert
have a side effect on JIT decisions? Even inRelease
builds? Is it mentioned somewhere or is it not guaranteed? What if the assertion no longer holds inRelease
?
Debug.Assert doesn't exist in Release code, it's Debug only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for clarification. I misunderstood your code comment, and thus the question arose.
Contributes to #94941
The safer patterns seem to show better codegen in fact + removed an allocation from
MapToIPv6