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

[API Proposal]: Int128 IPAddress.GetAddress() #83971

Closed
NN--- opened this issue Mar 27, 2023 · 14 comments
Closed

[API Proposal]: Int128 IPAddress.GetAddress() #83971

NN--- opened this issue Mar 27, 2023 · 14 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Milestone

Comments

@NN---
Copy link
Contributor

NN--- commented Mar 27, 2023

Background and motivation

Every call to IPAddrses.GetAddressBytes() allocates an array, while using IPAddress.Address only copies int.
The type System.Int128 can cover both IPv4 and IPv6 in a single call without GC pressure.

API Proposal

The property name to be defined.

namespace System.Net;

public class IPAddress
{
    ...
    public Int128 FullAddress { get; set; }
}

API Usage

var ipAddressV6 = IPAddress.Parse("::1");
Int128 fullAddressV6 = ipAddressV6.FullAddress

var ipAddressV4 = IPAddress.Parse("1.2.3.4");
Int128 fullAddressV4 = ipAddressV4.FullAddress

Alternative Designs

No simple alternative for this today.

Risks

Adding a new property should not be a high risk.

@NN--- NN--- added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 27, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 27, 2023
@ghost
Copy link

ghost commented Mar 27, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Every call to IPAddrses.GetAddressBytes() allocates an array, while using IPAddress.Address only copies int.
The type System.Int128 can cover both IPv4 and IPv6 in a single call without GC pressure.

API Proposal

The property name to be defined.

namespace System.Net;

public class IPAddress
{
    ...
    public Int128 FullAddress { get; set; }
}

API Usage

var ipAddressV6 = IPAddress.Parse("::1");
Int128 fullAddressV6 = ipAddressV6.FullAddress

var ipAddressV4 = IPAddress.Parse("1.2.3.4");
Int128 fullAddressV4 = ipAddressV4.FullAddress

Alternative Designs

No simple alternative for this today.

Risks

Adding a new property should not be a high risk.

Author: NN---
Assignees: -
Labels:

api-suggestion, area-System.Net

Milestone: -

@benaadams
Copy link
Member

Could also be a Vector128<byte> without worrying about endianness

@NN---
Copy link
Contributor Author

NN--- commented Mar 27, 2023

It can be in addition to Int128.
Endianess is not always a problem.

@gfoidl
Copy link
Member

gfoidl commented Mar 27, 2023

Could also be a Vector128<byte> without worrying about endianness

Quoting from #68328 (comment)

I'd prefer UInt128. The code doesn't require vectorization (it'll function fine if Vector128.IsHardwareAccelerated is false and we take a fallback path), and Vector128<T> looks scary.

@antonfirsov
Copy link
Member

antonfirsov commented Mar 27, 2023

Every call to IPAddrses.GetAddressBytes() allocates an array

We also have IPAddress.TryWriteBytes(..), which can write to any arbitrary destination buffer, regardless of the address family, which is more flexible than having a getter of a specific type.

If you need UInt128 (which you probably want here instead of the signed Int128) you can do:

UInt128 value = default; // or Unsafe.SkipInit(out UInt128 value)
bool success = address.TryWriteBytes(MemoryMarshal.AsBytes(new Span<UInt128>(ref value)), out int bytesWritten);

@NN--- have you considered using that method?

@NN---
Copy link
Contributor Author

NN--- commented Mar 27, 2023

@antonfirsov I see how it makes better compared to what we have today.
I guess the reason is for internal implementation of IPAddress which uses bytes[] _numbers, this why it cannot simply return an integer.
If the Int128 was stored internally, it would be more optimal to return it as a value instead of copying vectors.

@NN---
Copy link
Contributor Author

NN--- commented Mar 27, 2023

This is the code generated for TryWriteBytes:

;           Int128 i = 1;
;           a.TryWriteBytes(MemoryMarshal.AsBytes(new Span<Int128>(ref i)), out var written);
00007FFC357B4052  vmovdqu     xmm0,xmmword ptr [rbp-68h]  
00007FFC357B4057  vmovdqu     xmmword ptr [rbp-88h],xmm0  
00007FFC357B405F  lea         rdx,[rbp-88h]  
00007FFC357B4066  lea         rcx,[rbp-78h]  
00007FFC357B406A  call        qword ptr [CLRStub[MethodDescPrestub]@00007FFC359C7B10 (07FFC359C7B10h)]  
00007FFC357B4070  mov         rcx,qword ptr [rbp-58h]  
00007FFC357B4074  mov         qword ptr [rbp-0A0h],rcx  
00007FFC357B407B  vmovdqu     xmm0,xmmword ptr [rbp-78h]  
00007FFC357B4080  vmovdqu     xmmword ptr [rbp-98h],xmm0  
00007FFC357B4088  mov         rcx,qword ptr [rbp-0A0h]  
00007FFC357B408F  lea         rdx,[rbp-98h]  
00007FFC357B4096  lea         r8,[rbp-18h]  
00007FFC357B409A  cmp         dword ptr [rcx],ecx  
00007FFC357B409C  call        qword ptr [CLRStub[MethodDescPrestub]@00007FFC359C7570 (07FFC359C7570h)]  

While returning Int128 is much simpler:

;           Int128 i;
;           i = myIPAddress.FullAddress;
00007FFC357B4081  lea         rdx,[rbp-10h]  
00007FFC357B4085  mov         rcx,qword ptr [rbp-50h]  
00007FFC357B4089  cmp         dword ptr [rcx],ecx  
00007FFC357B408B  call        qword ptr [CLRStub[MethodDescPrestub]@00007FFC359C73D8 (07FFC359C73D8h)]  

@antonfirsov
Copy link
Member

antonfirsov commented Mar 27, 2023

This is the code generated for TryWriteBytes

Does this lead to measurable performance issues in your application? If yes we need to see more details to make the cause for potential improvements.

If the Int128 was stored internally, it would be more optimal to return it as a value instead of copying vectors.

That would penalize all instances of IPv4 IPAddress-es with 16-sizeof(IntPtr) extra bytes.

The main reason for TryWriteBytes's cost is that it has to reverse endianness. If we would do that in the constructor instead, TryWriteBytes could be implemented with a simple copy (with an increased cost for the ctr.). This is essentially true regardless if we store the data in a heap allocated array or in an embedded buffer eg. Int128.

@antonfirsov
Copy link
Member

antonfirsov commented Mar 30, 2023

@wfurt any thoughts here? If there are no strong counter-arguments, I recommend to close this as wontfix, since TryWriteBytes is a more flexible API. If anything, we should consider optimizing it by doing the endianness correction in the ctr. & maybe embedding the IPv6 buffer.

@NN---
Copy link
Contributor Author

NN--- commented Mar 30, 2023

Right now IPAddress stores an array of bytes which is GC object 16 bytes.
Trading it with Int128 leaves you with the same size.
I see your point about endianess, this has to be thought how to deal with it.
On the other hand we get less GC allocations for each IPv6 address which is nice.

It would be nice to have TryWriteBytes implemented closely as possible to simple Int128 return.

Thanks.

@antonfirsov
Copy link
Member

@NN--- is there any reason you prefer Int128 instead of eg. stackalloc byte[16]?

@NN---
Copy link
Contributor Author

NN--- commented Mar 31, 2023

TryWriteBytes is good enough, not as fast as returning a number directly but better than GetAddressBytes.

It seems that IPAddress is more optimized for IPv4 address.
For IPv4 address there is no heap allocation for byte[] _numbers member.

However, if this class is used for IPv6 address, there is always a heap allocation for byte[] _numbers.
Storing internally value as Int128 will minimize heap allocation for IPv6 addresses.

@wfurt
Copy link
Member

wfurt commented Apr 10, 2023

I would probably stay with TryWriteBytes for now. As far as I know you cannot pass Int128 to OS calls and IPAddress does not use it internally either. While that may change, it feels that we should not design public API on that assumptions.

And yes, IPAddress is probably IPv4 centric for historical reasons. We should not make same choice IMHO assuming 128 bits will be sufficient for all future addresses. (even if that seems reasonable at the moment)

Related to #30797, some of the troubles comes from allocation and GC afterwards. If anything I feel we could make argument for making IPAddress bigger to avoid separate allocation. DualMode Socket is the default now and it would be commonly used even for IPv4 destinations IMHO e.g. it would become IPv4 mapped IPv6 and allocate.

@antonfirsov
Copy link
Member

I'm closing this in favor of #84776, we don't need a new API.

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2023
@karelz karelz added this to the 8.0.0 milestone May 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Projects
None yet
Development

No branches or pull requests

6 participants