golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.81k stars 17.65k forks source link

x/net/ipv{4,6}: adopt net/netip address types #54883

Open matzf opened 2 years ago

matzf commented 2 years ago

The x/net/ipv{4,6} packages currently use net.IP and net.Addr types everywhere, with all the performance issues that the new types in the net/netip package were designed to resolve. In particular, the performance sensitive function ReadBatch must allocate the returned source addresses on every call (see related #26838).

In order to introduce this without breaking changes, parts of the API needs to be duplicated, analogous to, for example, net.ReadFromUDPAddrPort added in go-1.18. To avoid too much API bloat, it might be sufficient to implement this for performance sensitive functions like Read/WriteBatch.

Losely related to proposal #45886; when a fast net.Read/WriteUDPMsgs functionality is available, we may simply no longer need x/net.Read/WriteBatch for some use cases.

ianlancetaylor commented 2 years ago

CC @neild

rsc commented 2 years ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

rsc commented 2 years ago

What is the concrete API being proposed?

matzf commented 2 years ago

The minimal change to support netip.AddrPort for Read/WriteBatch in both x/net/ipv4 and x/net/ipv6:

type Message struct {
    Buffers [][]byte
    OOB     []byte
    Addr    net.Addr
    AddrPort    netip.AddrPort  // + Specifies destination address when writing with WriteAddrPort functions. Contains source address after reading with ReadAddrPort functions.
    N       int
    NN      int
    Flags   int
}

func (c *PacketConn) ReadBatchAddrPort(ms []Message, flags int) (int, error)
func (c *PacketConn) WriteBatchAddrPort(ms []Message, flags int) (int, error)
func (c *RawConn) ReadBatchAddrPort(ms []Message, flags int) (int, error)
func (c *RawConn) WriteBatchAddrPort(ms []Message, flags int) (int, error)

Note that for UDP, this almost identical to #45886. The remaining difference is the flags int parameter on the Read/WriteBatchAddrPort which does not exist on net.UDPConn.Read/WriteUDPMsgs.

Additionally, we could extend the ControlMessage type in the same style and duplicate the ReadFrom/WriteTo functions analogously. This would lead a lot of clutter though, and I'm not sure whether there will be enough of a performance boost to justify this bloat.

An alternative option could be to add a new package, e.g. x/net/ip, replacing the ipv4 and ipv6 packages with a new API based solely around netip.Addr and AddrPort. If this seems like a useful idea, I can work out a detailed proposal for this.

rsc commented 1 year ago

If we pattern off of #45886, maybe we should define IPMsg matching the new UDPMsg (using its field names, not the ones in ipv4.Message) and add ReadIPMsgs and WriteIPMsgs methods taking []IPMsg?

Then at least people using the UDP version would know how to use the IP version and vice versa? Also then there's not two different addresses in Message to worry about. We'd also be able to drop the flags arguments, since the flags field is in IPMsg.

rsc commented 1 year ago

@martin-sucha @database64128 you commented on #45886. Any thoughts about my previous comment, essentially applying the same API here?

rsc commented 1 year ago

If we apply the #45886 API then we get

type IPMsg struct {
  Buffer  [][]byte // must be pre-allocated by caller to non-zero len; callee fills, re-slices elements
  Control [][]byte // like Buffer (pre-allocated, re-sliced), may be zero-length
  Addr    netip.AddrPort

  // Flags is an OS-specific bitmask.
  Flags int
}

func (c *PacketConn) ReadIPMsgs(ms []IPMsg, flags int) (msgsRead int, err error)
func (c *PacketConn) WriteIPMsgs(ms []IPMsg, flags int) (msgsSent int, err error)
func (c *RawConn) ReadIPMsgs(ms []IPMsg, flags int) (msgsRead int, err error)
func (c *RawConn) WriteIPMsgs(ms []IPMsg, flags int) (msgsSent int, err error)

Do I have that right? Does that sound like a reasonable API?

matzf commented 1 year ago

The comment // populated by callee on IPMsg.Addr does not seem right for the write APIs.

I see that the there is no int flags arguments on the Read/Write functions, as you had proposed further up:

We'd also be able to drop the flags arguments, since the flags field is in IPMsg.

Note that the C-API has a flags field in the message header and a flags parameter for the send/receive functions. The flags field in the message header is only used to return flags on received messages. To set per-call options, like MSG_OOB, MSG_MORE, MSG_DONTWAIT, the flags parameter is used. Now we could of course say that we use the flags field of the first message to set these per-call options, but I'm not sure that we're really gaining much from this.

Otherwise, this looks great to me, thanks for taking this up!

rsc commented 1 year ago

Add flags bits to the argument list and removed the populated by callee comment. Any other comments on the API? Does anyone object to adding this API?

rsc commented 1 year ago

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

rsc commented 1 year ago

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group